| Message ID | 6051153.b6TE1dlrel@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Wednesday, July 24, 2013 02:05:15 PM Linus Torvalds wrote: >> On Wed, Jul 24, 2013 at 2:02 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> > >> > I think a i915 module option should be doable, otoh people seem to have a >> > viable workaround by setting a different acpi os version already. >> >> At least the original claim was that if you set a non-windows8 acpi os >> version, you hit other bugs.. >> >> But yeah, if we just do a plain revert, that may be the only option >> for the people for whom the current (to-be-reverted) patches made >> things work. > > Well, I wonder what about the appended (untested) patch? Rafael, before going there, I've been trying to wrap my (poor, rusty after vacation) head around commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Date: Thu Jul 18 02:08:06 2013 +0200 ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 and I can't see how it could work. First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before it's actually set anywhere. Buried deep in the calls from acpi_video_bus_add(), acpi_video_verify_backlight_support() is used before acpi_video_backlight_quirks() gets called. (Perhaps if i915 is reloaded, this goes right as the flags are already set.) Second, with i915 that has opregion support, __acpi_video_register() should only ever get called once. Which means the acpi_walk_namespace() with video_unregister_backlight() should never get called in register. Please enlighten me. BR, Jani.
On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: > On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > On Wednesday, July 24, 2013 02:05:15 PM Linus Torvalds wrote: > >> On Wed, Jul 24, 2013 at 2:02 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > >> > > >> > I think a i915 module option should be doable, otoh people seem to have a > >> > viable workaround by setting a different acpi os version already. > >> > >> At least the original claim was that if you set a non-windows8 acpi os > >> version, you hit other bugs.. > >> > >> But yeah, if we just do a plain revert, that may be the only option > >> for the people for whom the current (to-be-reverted) patches made > >> things work. > > > > Well, I wonder what about the appended (untested) patch? > > Rafael, before going there, I've been trying to wrap my (poor, rusty > after vacation) head around > > commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Date: Thu Jul 18 02:08:06 2013 +0200 > > ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 > > and I can't see how it could work. Well, if it didn't work, people wouldn't see either improvement or breakage from it, but they do see that, so it evidently works. :-) > First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before > it's actually set anywhere. Are you sure about that? acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which in fact is an ACPI driver (the naming sucks, but I didn't invent it). This means that acpi_video_bus_add() can only be called *after* acpi_video_bus has been registered with the ACPI subsystem (and the driver core). That is done by acpi_bus_register_driver() and, guess what?, this happens in __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before __acpi_video_register(). > Buried deep in the calls from > acpi_video_bus_add(), acpi_video_verify_backlight_support() is used > before acpi_video_backlight_quirks() gets called. (Perhaps if i915 is > reloaded, this goes right as the flags are already set.) Please see above. > Second, with i915 that has opregion support, __acpi_video_register() > should only ever get called once. Which means the acpi_walk_namespace() > with video_unregister_backlight() should never get called in register. > > Please enlighten me. Actually, that's correct, so we don't need the whole video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would be sufficient. Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave acpi_video_backlight_quirks() as is so that it can be used by acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause problems to happen. Thanks, Rafael
On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: >> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: >> > Well, I wonder what about the appended (untested) patch? >> >> Rafael, before going there, I've been trying to wrap my (poor, rusty >> after vacation) head around >> >> commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 >> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Date: Thu Jul 18 02:08:06 2013 +0200 >> >> ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 >> >> and I can't see how it could work. > > Well, if it didn't work, people wouldn't see either improvement or breakage > from it, but they do see that, so it evidently works. :-) I didn't claim it didn't work, just that *I* didn't see how it could. ;) >> First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before >> it's actually set anywhere. > > Are you sure about that? > > acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which > in fact is an ACPI driver (the naming sucks, but I didn't invent it). This > means that acpi_video_bus_add() can only be called *after* acpi_video_bus > has been registered with the ACPI subsystem (and the driver core). That > is done by acpi_bus_register_driver() and, guess what?, this happens in > __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before > __acpi_video_register(). Right. I totally missed the call within the ternary operator. Thanks for the explanation, and apologies for the noise. >> Second, with i915 that has opregion support, __acpi_video_register() >> should only ever get called once. Which means the acpi_walk_namespace() >> with video_unregister_backlight() should never get called in register. >> >> Please enlighten me. > > Actually, that's correct, so we don't need the whole > video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would > be sufficient. > > Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave > acpi_video_backlight_quirks() as is so that it can be used by > acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause > problems to happen. I observe that for the regular non-quirk acpi_video_register() call, acpi_video_backlight_quirks() won't be called during register, but it will get called later. This might have subtle effects later on, don't you think? As to the original problem, and your patch in this thread, what do you think about having another value in acpi_backlight kernel parameter for it? Having an i915 module parameter to tell acpi to use or not use quirks seems odd, since the i915 is not really taking over anything. It's just passing the info on to acpi. BR, Jani.
On Thursday, July 25, 2013 03:34:10 PM Jani Nikula wrote: > On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: > >> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > >> > Well, I wonder what about the appended (untested) patch? > >> > >> Rafael, before going there, I've been trying to wrap my (poor, rusty > >> after vacation) head around > >> > >> commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 > >> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> Date: Thu Jul 18 02:08:06 2013 +0200 > >> > >> ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 > >> > >> and I can't see how it could work. > > > > Well, if it didn't work, people wouldn't see either improvement or breakage > > from it, but they do see that, so it evidently works. :-) > > I didn't claim it didn't work, just that *I* didn't see how it could. ;) > > >> First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before > >> it's actually set anywhere. > > > > Are you sure about that? > > > > acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which > > in fact is an ACPI driver (the naming sucks, but I didn't invent it). This > > means that acpi_video_bus_add() can only be called *after* acpi_video_bus > > has been registered with the ACPI subsystem (and the driver core). That > > is done by acpi_bus_register_driver() and, guess what?, this happens in > > __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before > > __acpi_video_register(). > > Right. I totally missed the call within the ternary operator. Thanks for > the explanation, and apologies for the noise. > > >> Second, with i915 that has opregion support, __acpi_video_register() > >> should only ever get called once. Which means the acpi_walk_namespace() > >> with video_unregister_backlight() should never get called in register. > >> > >> Please enlighten me. > > > > Actually, that's correct, so we don't need the whole > > video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would > > be sufficient. > > > > Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave > > acpi_video_backlight_quirks() as is so that it can be used by > > acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause > > problems to happen. > > I observe that for the regular non-quirk acpi_video_register() call, > acpi_video_backlight_quirks() won't be called during register, but it > will get called later. This might have subtle effects later on, don't > you think? Yes, it might, but after dropping ACPI_VIDEO_SKIP_BACKLIGHT it should be OK. > As to the original problem, and your patch in this thread, what do you > think about having another value in acpi_backlight kernel parameter for > it? Having an i915 module parameter to tell acpi to use or not use > quirks seems odd, since the i915 is not really taking over > anything. It's just passing the info on to acpi. I agree, I'm going to send a full revert in a while and we'll think what to do about all that later. Thanks, Rafael
Hi Rafael, did you commit a full revert? Because I am experiencing quite weird things in rc3. Do we have a bug opened to discuss about it? Here is what I can observe: 1) During boot, probably when loading the driver, backlight gets off (or to a level low enough to make me feel it is off) 2) When I am playing with my Fn+x keys, I am getting a completely full / completely low brightness with no intermediate steps 3) When I am playing with my Fn+x keys while gnome brightness settings panel is open, I am recovering intermediate steps but the Fn+x keys behavior is inverted (the key supposed to lower the brightness make it increase and vice-versa. Note that the gnome brightness indicator also gets inverted). 4) Playing with the mouse on gnome brightness settings is working, except that on the minimum level, backlight gets off 5) Writing to /sys/class/backlight/intel_backlight/brightness works Regards On 07/25/2013 02:57 PM, Rafael J. Wysocki wrote: > On Thursday, July 25, 2013 03:34:10 PM Jani Nikula wrote: >> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: >>> On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: >>>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: >>>>> Well, I wonder what about the appended (untested) patch? >>>> Rafael, before going there, I've been trying to wrap my (poor, rusty >>>> after vacation) head around >>>> >>>> commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 >>>> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>> Date: Thu Jul 18 02:08:06 2013 +0200 >>>> >>>> ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 >>>> >>>> and I can't see how it could work. >>> Well, if it didn't work, people wouldn't see either improvement or breakage >>> from it, but they do see that, so it evidently works. :-) >> I didn't claim it didn't work, just that *I* didn't see how it could. ;) >> >>>> First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before >>>> it's actually set anywhere. >>> Are you sure about that? >>> >>> acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which >>> in fact is an ACPI driver (the naming sucks, but I didn't invent it). This >>> means that acpi_video_bus_add() can only be called *after* acpi_video_bus >>> has been registered with the ACPI subsystem (and the driver core). That >>> is done by acpi_bus_register_driver() and, guess what?, this happens in >>> __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before >>> __acpi_video_register(). >> Right. I totally missed the call within the ternary operator. Thanks for >> the explanation, and apologies for the noise. >> >>>> Second, with i915 that has opregion support, __acpi_video_register() >>>> should only ever get called once. Which means the acpi_walk_namespace() >>>> with video_unregister_backlight() should never get called in register. >>>> >>>> Please enlighten me. >>> Actually, that's correct, so we don't need the whole >>> video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would >>> be sufficient. >>> >>> Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave >>> acpi_video_backlight_quirks() as is so that it can be used by >>> acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause >>> problems to happen. >> I observe that for the regular non-quirk acpi_video_register() call, >> acpi_video_backlight_quirks() won't be called during register, but it >> will get called later. This might have subtle effects later on, don't >> you think? > Yes, it might, but after dropping ACPI_VIDEO_SKIP_BACKLIGHT it should be OK. > >> As to the original problem, and your patch in this thread, what do you >> think about having another value in acpi_backlight kernel parameter for >> it? Having an i915 module parameter to tell acpi to use or not use >> quirks seems odd, since the i915 is not really taking over >> anything. It's just passing the info on to acpi. > I agree, I'm going to send a full revert in a while and we'll think what to > do about all that later. > > Thanks, > Rafael > >
On Monday, July 29, 2013 09:36:31 PM * SAMÍ * wrote: > Hi Rafael, > > > did you commit a full revert? Yes, but I left acpi_video_backlight_quirks() (in drivers/acpi/video_detect.c) that is used to decide what to do with _DOS. Can you please check if making that function always return 'false' makes any difference? Rafael > Because I am experiencing quite weird things in rc3. > Do we have a bug opened to discuss about it? > > Here is what I can observe: > 1) During boot, probably when loading the driver, backlight gets off (or > to a level low enough to make me feel it is off) > 2) When I am playing with my Fn+x keys, I am getting a completely full / > completely low brightness with no intermediate steps > 3) When I am playing with my Fn+x keys while gnome brightness settings > panel is open, I am recovering intermediate steps but the Fn+x keys > behavior is inverted (the key supposed to lower the brightness make it > increase and vice-versa. Note that the gnome brightness indicator also > gets inverted). > 4) Playing with the mouse on gnome brightness settings is working, > except that on the minimum level, backlight gets off > 5) Writing to /sys/class/backlight/intel_backlight/brightness works > > > Regards > > On 07/25/2013 02:57 PM, Rafael J. Wysocki wrote: > > On Thursday, July 25, 2013 03:34:10 PM Jani Nikula wrote: > >> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > >>> On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: > >>>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > >>>>> Well, I wonder what about the appended (untested) patch? > >>>> Rafael, before going there, I've been trying to wrap my (poor, rusty > >>>> after vacation) head around > >>>> > >>>> commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 > >>>> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>>> Date: Thu Jul 18 02:08:06 2013 +0200 > >>>> > >>>> ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 > >>>> > >>>> and I can't see how it could work. > >>> Well, if it didn't work, people wouldn't see either improvement or breakage > >>> from it, but they do see that, so it evidently works. :-) > >> I didn't claim it didn't work, just that *I* didn't see how it could. ;) > >> > >>>> First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before > >>>> it's actually set anywhere. > >>> Are you sure about that? > >>> > >>> acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which > >>> in fact is an ACPI driver (the naming sucks, but I didn't invent it). This > >>> means that acpi_video_bus_add() can only be called *after* acpi_video_bus > >>> has been registered with the ACPI subsystem (and the driver core). That > >>> is done by acpi_bus_register_driver() and, guess what?, this happens in > >>> __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before > >>> __acpi_video_register(). > >> Right. I totally missed the call within the ternary operator. Thanks for > >> the explanation, and apologies for the noise. > >> > >>>> Second, with i915 that has opregion support, __acpi_video_register() > >>>> should only ever get called once. Which means the acpi_walk_namespace() > >>>> with video_unregister_backlight() should never get called in register. > >>>> > >>>> Please enlighten me. > >>> Actually, that's correct, so we don't need the whole > >>> video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would > >>> be sufficient. > >>> > >>> Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave > >>> acpi_video_backlight_quirks() as is so that it can be used by > >>> acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause > >>> problems to happen. > >> I observe that for the regular non-quirk acpi_video_register() call, > >> acpi_video_backlight_quirks() won't be called during register, but it > >> will get called later. This might have subtle effects later on, don't > >> you think? > > Yes, it might, but after dropping ACPI_VIDEO_SKIP_BACKLIGHT it should be OK. > > > >> As to the original problem, and your patch in this thread, what do you > >> think about having another value in acpi_backlight kernel parameter for > >> it? Having an i915 module parameter to tell acpi to use or not use > >> quirks seems odd, since the i915 is not really taking over > >> anything. It's just passing the info on to acpi. > > I agree, I'm going to send a full revert in a while and we'll think what to > > do about all that later. > > > > Thanks, > > Rafael > > > > >
When I make acpi_video_backlight_quirks() return false: - the Fn+x keys are not working anymore (remember that they didn't work in 3.10 nor 3.9) - At least the backlight remains on at boot. - Gnome brightness settings do not work anymore. Neither do xbacklight. - Writing to /sys/class/backlight/intel_backlight/brightness works Regards, Sam On 07/29/2013 10:03 PM, Rafael J. Wysocki wrote: > On Monday, July 29, 2013 09:36:31 PM * SAMÍ * wrote: >> Hi Rafael, >> >> >> did you commit a full revert? > Yes, but I left acpi_video_backlight_quirks() (in drivers/acpi/video_detect.c) > that is used to decide what to do with _DOS. > > Can you please check if making that function always return 'false' makes any > difference? > > Rafael > > >> Because I am experiencing quite weird things in rc3. >> Do we have a bug opened to discuss about it? >> >> Here is what I can observe: >> 1) During boot, probably when loading the driver, backlight gets off (or >> to a level low enough to make me feel it is off) >> 2) When I am playing with my Fn+x keys, I am getting a completely full / >> completely low brightness with no intermediate steps >> 3) When I am playing with my Fn+x keys while gnome brightness settings >> panel is open, I am recovering intermediate steps but the Fn+x keys >> behavior is inverted (the key supposed to lower the brightness make it >> increase and vice-versa. Note that the gnome brightness indicator also >> gets inverted). >> 4) Playing with the mouse on gnome brightness settings is working, >> except that on the minimum level, backlight gets off >> 5) Writing to /sys/class/backlight/intel_backlight/brightness works >> >> >> Regards >> >> On 07/25/2013 02:57 PM, Rafael J. Wysocki wrote: >>> On Thursday, July 25, 2013 03:34:10 PM Jani Nikula wrote: >>>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: >>>>> On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: >>>>>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: >>>>>>> Well, I wonder what about the appended (untested) patch? >>>>>> Rafael, before going there, I've been trying to wrap my (poor, rusty >>>>>> after vacation) head around >>>>>> >>>>>> commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 >>>>>> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>> Date: Thu Jul 18 02:08:06 2013 +0200 >>>>>> >>>>>> ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 >>>>>> >>>>>> and I can't see how it could work. >>>>> Well, if it didn't work, people wouldn't see either improvement or breakage >>>>> from it, but they do see that, so it evidently works. :-) >>>> I didn't claim it didn't work, just that *I* didn't see how it could. ;) >>>> >>>>>> First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before >>>>>> it's actually set anywhere. >>>>> Are you sure about that? >>>>> >>>>> acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which >>>>> in fact is an ACPI driver (the naming sucks, but I didn't invent it). This >>>>> means that acpi_video_bus_add() can only be called *after* acpi_video_bus >>>>> has been registered with the ACPI subsystem (and the driver core). That >>>>> is done by acpi_bus_register_driver() and, guess what?, this happens in >>>>> __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before >>>>> __acpi_video_register(). >>>> Right. I totally missed the call within the ternary operator. Thanks for >>>> the explanation, and apologies for the noise. >>>> >>>>>> Second, with i915 that has opregion support, __acpi_video_register() >>>>>> should only ever get called once. Which means the acpi_walk_namespace() >>>>>> with video_unregister_backlight() should never get called in register. >>>>>> >>>>>> Please enlighten me. >>>>> Actually, that's correct, so we don't need the whole >>>>> video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would >>>>> be sufficient. >>>>> >>>>> Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave >>>>> acpi_video_backlight_quirks() as is so that it can be used by >>>>> acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause >>>>> problems to happen. >>>> I observe that for the regular non-quirk acpi_video_register() call, >>>> acpi_video_backlight_quirks() won't be called during register, but it >>>> will get called later. This might have subtle effects later on, don't >>>> you think? >>> Yes, it might, but after dropping ACPI_VIDEO_SKIP_BACKLIGHT it should be OK. >>> >>>> As to the original problem, and your patch in this thread, what do you >>>> think about having another value in acpi_backlight kernel parameter for >>>> it? Having an i915 module parameter to tell acpi to use or not use >>>> quirks seems odd, since the i915 is not really taking over >>>> anything. It's just passing the info on to acpi. >>> I agree, I'm going to send a full revert in a while and we'll think what to >>> do about all that later. >>> >>> Thanks, >>> Rafael >>> >>>
On Monday, July 29, 2013 11:53:59 PM * SAMÍ * wrote: > When I make acpi_video_backlight_quirks() return false: > - the Fn+x keys are not working anymore (remember that they didn't work > in 3.10 nor 3.9) > - At least the backlight remains on at boot. > - Gnome brightness settings do not work anymore. Neither do xbacklight. > - Writing to /sys/class/backlight/intel_backlight/brightness works Well, you're better off with acpi_video_backlight_quirks() as is, then. :-) I'm afraid we can't help you by revering anything more at this point. Please file a bug in the kernel BZ to further track the issues you're seeing. Thanks, Rafael > On 07/29/2013 10:03 PM, Rafael J. Wysocki wrote: > > On Monday, July 29, 2013 09:36:31 PM * SAMÍ * wrote: > >> Hi Rafael, > >> > >> > >> did you commit a full revert? > > Yes, but I left acpi_video_backlight_quirks() (in drivers/acpi/video_detect.c) > > that is used to decide what to do with _DOS. > > > > Can you please check if making that function always return 'false' makes any > > difference? > > > > Rafael > > > > > >> Because I am experiencing quite weird things in rc3. > >> Do we have a bug opened to discuss about it? > >> > >> Here is what I can observe: > >> 1) During boot, probably when loading the driver, backlight gets off (or > >> to a level low enough to make me feel it is off) > >> 2) When I am playing with my Fn+x keys, I am getting a completely full / > >> completely low brightness with no intermediate steps > >> 3) When I am playing with my Fn+x keys while gnome brightness settings > >> panel is open, I am recovering intermediate steps but the Fn+x keys > >> behavior is inverted (the key supposed to lower the brightness make it > >> increase and vice-versa. Note that the gnome brightness indicator also > >> gets inverted). > >> 4) Playing with the mouse on gnome brightness settings is working, > >> except that on the minimum level, backlight gets off > >> 5) Writing to /sys/class/backlight/intel_backlight/brightness works > >> > >> > >> Regards > >> > >> On 07/25/2013 02:57 PM, Rafael J. Wysocki wrote: > >>> On Thursday, July 25, 2013 03:34:10 PM Jani Nikula wrote: > >>>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > >>>>> On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: > >>>>>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > >>>>>>> Well, I wonder what about the appended (untested) patch? > >>>>>> Rafael, before going there, I've been trying to wrap my (poor, rusty > >>>>>> after vacation) head around > >>>>>> > >>>>>> commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 > >>>>>> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>>>>> Date: Thu Jul 18 02:08:06 2013 +0200 > >>>>>> > >>>>>> ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 > >>>>>> > >>>>>> and I can't see how it could work. > >>>>> Well, if it didn't work, people wouldn't see either improvement or breakage > >>>>> from it, but they do see that, so it evidently works. :-) > >>>> I didn't claim it didn't work, just that *I* didn't see how it could. ;) > >>>> > >>>>>> First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before > >>>>>> it's actually set anywhere. > >>>>> Are you sure about that? > >>>>> > >>>>> acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which > >>>>> in fact is an ACPI driver (the naming sucks, but I didn't invent it). This > >>>>> means that acpi_video_bus_add() can only be called *after* acpi_video_bus > >>>>> has been registered with the ACPI subsystem (and the driver core). That > >>>>> is done by acpi_bus_register_driver() and, guess what?, this happens in > >>>>> __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before > >>>>> __acpi_video_register(). > >>>> Right. I totally missed the call within the ternary operator. Thanks for > >>>> the explanation, and apologies for the noise. > >>>> > >>>>>> Second, with i915 that has opregion support, __acpi_video_register() > >>>>>> should only ever get called once. Which means the acpi_walk_namespace() > >>>>>> with video_unregister_backlight() should never get called in register. > >>>>>> > >>>>>> Please enlighten me. > >>>>> Actually, that's correct, so we don't need the whole > >>>>> video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would > >>>>> be sufficient. > >>>>> > >>>>> Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave > >>>>> acpi_video_backlight_quirks() as is so that it can be used by > >>>>> acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause > >>>>> problems to happen. > >>>> I observe that for the regular non-quirk acpi_video_register() call, > >>>> acpi_video_backlight_quirks() won't be called during register, but it > >>>> will get called later. This might have subtle effects later on, don't > >>>> you think? > >>> Yes, it might, but after dropping ACPI_VIDEO_SKIP_BACKLIGHT it should be OK. > >>> > >>>> As to the original problem, and your patch in this thread, what do you > >>>> think about having another value in acpi_backlight kernel parameter for > >>>> it? Having an i915 module parameter to tell acpi to use or not use > >>>> quirks seems odd, since the i915 is not really taking over > >>>> anything. It's just passing the info on to acpi. > >>> I agree, I'm going to send a full revert in a while and we'll think what to > >>> do about all that later. > >>> > >>> Thanks, > >>> Rafael > >>> > >>> >
On 07/30/2013 03:36 AM, * SAMÍ * wrote: > Hi Rafael, > > > did you commit a full revert? > Because I am experiencing quite weird things in rc3. > Do we have a bug opened to discuss about it? Yes we have: https://bugzilla.kernel.org/show_bug.cgi?id=52951 I'll look into this issue. Thanks, Aaron > > Here is what I can observe: > 1) During boot, probably when loading the driver, backlight gets off (or > to a level low enough to make me feel it is off) > 2) When I am playing with my Fn+x keys, I am getting a completely full / > completely low brightness with no intermediate steps > 3) When I am playing with my Fn+x keys while gnome brightness settings > panel is open, I am recovering intermediate steps but the Fn+x keys > behavior is inverted (the key supposed to lower the brightness make it > increase and vice-versa. Note that the gnome brightness indicator also > gets inverted). > 4) Playing with the mouse on gnome brightness settings is working, > except that on the minimum level, backlight gets off > 5) Writing to /sys/class/backlight/intel_backlight/brightness works > > > Regards > > On 07/25/2013 02:57 PM, Rafael J. Wysocki wrote: >> On Thursday, July 25, 2013 03:34:10 PM Jani Nikula wrote: >>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: >>>> On Thursday, July 25, 2013 11:09:27 AM Jani Nikula wrote: >>>>> On Thu, 25 Jul 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote: >>>>>> Well, I wonder what about the appended (untested) patch? >>>>> Rafael, before going there, I've been trying to wrap my (poor, rusty >>>>> after vacation) head around >>>>> >>>>> commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255 >>>>> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>> Date: Thu Jul 18 02:08:06 2013 +0200 >>>>> >>>>> ACPI / video / i915: No ACPI backlight if firmware expects Windows 8 >>>>> >>>>> and I can't see how it could work. >>>> Well, if it didn't work, people wouldn't see either improvement or breakage >>>> from it, but they do see that, so it evidently works. :-) >>> I didn't claim it didn't work, just that *I* didn't see how it could. ;) >>> >>>>> First, the ACPI_VIDEO_SKIP_BACKLIGHT flag seems to be checked before >>>>> it's actually set anywhere. >>>> Are you sure about that? >>>> >>>> acpi_video_bus_add() is the .add() callback routine for acpi_video_bus which >>>> in fact is an ACPI driver (the naming sucks, but I didn't invent it). This >>>> means that acpi_video_bus_add() can only be called *after* acpi_video_bus >>>> has been registered with the ACPI subsystem (and the driver core). That >>>> is done by acpi_bus_register_driver() and, guess what?, this happens in >>>> __acpi_video_register(). So clearly, acpi_video_bus_add() *cannot* run before >>>> __acpi_video_register(). >>> Right. I totally missed the call within the ternary operator. Thanks for >>> the explanation, and apologies for the noise. >>> >>>>> Second, with i915 that has opregion support, __acpi_video_register() >>>>> should only ever get called once. Which means the acpi_walk_namespace() >>>>> with video_unregister_backlight() should never get called in register. >>>>> >>>>> Please enlighten me. >>>> Actually, that's correct, so we don't need the whole >>>> video_unregister_backlight() thing, calling acpi_video_backlight_quirks() would >>>> be sufficient. >>>> >>>> Ah, one more reason to do a full revert. I'm thinking, though, that I'll leave >>>> acpi_video_backlight_quirks() as is so that it can be used by >>>> acpi_video_bus_(start)|(stop)_devices(), because that doesn't seem to cause >>>> problems to happen. >>> I observe that for the regular non-quirk acpi_video_register() call, >>> acpi_video_backlight_quirks() won't be called during register, but it >>> will get called later. This might have subtle effects later on, don't >>> you think? >> Yes, it might, but after dropping ACPI_VIDEO_SKIP_BACKLIGHT it should be OK. >> >>> As to the original problem, and your patch in this thread, what do you >>> think about having another value in acpi_backlight kernel parameter for >>> it? Having an i915 module parameter to tell acpi to use or not use >>> quirks seems odd, since the i915 is not really taking over >>> anything. It's just passing the info on to acpi. >> I agree, I'm going to send a full revert in a while and we'll think what to >> do about all that later. >> >> Thanks, >> Rafael >> >> >
Index: linux-pm/drivers/gpu/drm/i915/i915_dma.c =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -1648,7 +1648,7 @@ int i915_driver_load(struct drm_device * if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev); - acpi_video_register_with_quirks(); + __acpi_video_register(i915_take_over_backlight); } if (IS_GEN5(dev)) Index: linux-pm/drivers/gpu/drm/i915/i915_drv.c =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_drv.c +++ linux-pm/drivers/gpu/drm/i915/i915_drv.c @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1; module_param_named(enable_ips, i915_enable_ips, int, 0600); MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)"); +bool i915_take_over_backlight __read_mostly = false; +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644); +MODULE_PARM_DESC(take_over_backlight, + "Prevent ACPI backlight from being used (default: false)"); + static struct drm_driver driver; extern int intel_agp_enabled; Index: linux-pm/drivers/gpu/drm/i915/i915_drv.h =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_drv.h +++ linux-pm/drivers/gpu/drm/i915/i915_drv.h @@ -1542,6 +1542,7 @@ extern int i915_enable_ppgtt __read_most extern unsigned int i915_preliminary_hw_support __read_mostly; extern int i915_disable_power_well __read_mostly; extern int i915_enable_ips __read_mostly; +extern bool i915_take_over_backlight __read_mostly; extern int i915_suspend(struct drm_device *dev, pm_message_t state); extern int i915_resume(struct drm_device *dev); Index: linux-pm/include/acpi/video.h =================================================================== --- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -18,20 +18,11 @@ struct acpi_device; #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) extern int __acpi_video_register(bool backlight_quirks); -static inline int acpi_video_register(void) -{ - return __acpi_video_register(false); -} -static inline int acpi_video_register_with_quirks(void) -{ - return __acpi_video_register(true); -} extern void acpi_video_unregister(void); extern int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid); #else -static inline int acpi_video_register(void) { return 0; } -static inline int acpi_video_register_with_quirks(void) { return 0; } +static inline int __acpi_video_register(bool backlight_quirks) { return 0; } static inline void acpi_video_unregister(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid) @@ -40,4 +31,9 @@ static inline int acpi_video_get_edid(st } #endif +static inline int acpi_video_register(void) +{ + return __acpi_video_register(false); +} + #endif