Re: Linux 3.11-rc2 [backlight] [ASUS N56VZ]

Re: Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Rafael J. Wysocki]

On Wednesday, July 24, 2013 08:43:00 PM * SAMÍ * wrote:
> Hi,
> 
> it seems a lot of people are complaining about the backlight pull 
> request of RC2.
> 
> This is to tell you how it saved my life:
> - Changing brightness from Fn keys does work
> - Writing to /sys/class/backlight/intel_backlight/brightness does work
> - Changing brightness from gnome brightness settings does work
> --> It seems everything is now working (it wasn't before).
> --> I appreciate your patch and I am going to be sad if you revert it :-)

Well, there seems to be a number of people who'll be equally unhappy if I don't
do that.  Unfortunately for you, things work for them without that patch.

The source of the problem seems to be that the i915's backlight support doesn't
work for all those people, although it should work anyway in principle.  So to
me the way to go seems to be to revert the patch for now, fix the i915
backlight interface for them and try to apply it again when we're more
confident about the i915 backlight.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Linus Torvalds]

On Wed, Jul 24, 2013 at 12:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> Well, there seems to be a number of people who'll be equally unhappy if I don't
> do that.  Unfortunately for you, things work for them without that patch.

So I think we have to revert the behavior back, but I wonder if we
could keep the logic and have a i915 kernel command line option to
enable it.

We did that for a long time with "drm.modeset" and the
"i915.enable_rc6" mess. We still have the "i915.invert_brightness"
thing, although I don't know who actually uses it. But that option is
itself an indication that the i915 driver still has some backlight
issues.

But if that kind of "revert behavior" isn't easy, we need to just
revert the patches entirely.

               Linus

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Daniel Vetter]

On Wed, Jul 24, 2013 at 10:39 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Wed, Jul 24, 2013 at 12:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> Well, there seems to be a number of people who'll be equally unhappy if I don't
>> do that.  Unfortunately for you, things work for them without that patch.
>
> So I think we have to revert the behavior back, but I wonder if we
> could keep the logic and have a i915 kernel command line option to
> enable it.
>
> We did that for a long time with "drm.modeset" and the
> "i915.enable_rc6" mess. We still have the "i915.invert_brightness"
> thing, although I don't know who actually uses it. But that option is
> itself an indication that the i915 driver still has some backlight
> issues.

invert_brighntess seems to be real, at least we have a pile of quirk
entries for Acer and Packard Bell machines. Generally I'm not too happy
with module options since they tend to get stuck eternally in a limbo
state where everyone in the know sets them to their liking and no one
bothers to fix the underlying.

Otoh this is the backlight and I've essentially given up and opted for
quirk entries :(

> But if that kind of "revert behavior" isn't easy, we need to just
> revert the patches entirely.

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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Linus Torvalds]

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.

            Linus

Re: Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Rafael J. Wysocki]

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?

It should restore the previous behavior while leaving an option to use the
new stuff if need be.

Rafael


---
 drivers/gpu/drm/i915/i915_dma.c |    2 +-
 drivers/gpu/drm/i915/i915_drv.c |    5 +++++
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 include/acpi/video.h            |   16 ++++++----------
 4 files changed, 13 insertions(+), 11 deletions(-)

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

Re: Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Jani Nikula]

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.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Rafael J. Wysocki]

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


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

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Jani Nikula]

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.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Rafael J. Wysocki]

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


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

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[* SAMÍ *]

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

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Rafael J. Wysocki]

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
> >
> >
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[* SAMÍ *]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Rafael J. Wysocki]

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
> >>>
> >>>
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Intel-gfx] Linux 3.11-rc2 [backlight] [ASUS N56VZ]

[Aaron Lu]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html