[Intel-gfx] [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
> On Mon, 09 Sep 2013, "Rafael J. Wysocki" <rjw at sisk.pl> wrote:
> > On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
> >> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> >> > Hi,
> >> > 
> >> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> >> > > Hi Aaaron,
> >> > > 
> >> > > Have we grown any clue meanwhile about which Intel boxes need this and for
> >> > > which we still need to keep the acpi backlight around?
> >> > 
> >> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> >> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
> >> > with Win8, the backlight AML goes into a special code path that is broken on
> >> > those machines. Presumably Win8 uses native backlight control on them and that
> >> > AML code path is never executed there.]  The collection of machines with this
> >> > problem appears to be kind of random (various models from various vendors), but
> >> > I *think* they are machines that originally shipped with Win7 with a Win8
> >> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
> >> > 
> >> > Because of that, last time we tried to switch all of the systems using i915
> >> > and telling the BIOS that they are compatible with Win8 over to native backlight
> >> > control, but that didn't work for two reasons.  The first reason is that some
> >> > user space doesn't know how to use intel_backlight and needs to be taught about
> >> > that (so some systems are just not ready for that switch).  The second and more
> >> > fundamental reason is that the native backlight control simply doesn't work on
> >> > some machines and we don't seem to have any idea why and how to debug this
> >> > particular problem (mostly because we don't have enough information and we
> >> > don't know what to ask for).
> >> > 
> >> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> >> > > the backlight.
> >> > >
> >> > > Almost all the quirks we have added recently (or that have been proposed
> >> > > to be added) are for the backlight. Imo that indicates we get something
> >> > > fundamentally wrong ...
> >> > 
> >> > This thing isn't really a quirk.  It rather is an option for the users of
> >> > the systems where ACPI backlight doesn't work to switch over to the native
> >> > backlight control using a command line switch.  This way they can at least
> >> > *see* if the native backlight control works for them and (hopefully) report
> >> > problems if that's not the case.  This gives us a chance to get more
> >> > information about what the problem is and possibly to make some progress
> >> > without making changes for everyone, reverting those changes when they don't
> >> > work etc.
> >> > 
> >> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> >> > make the ACPI backlight work for them again, but this rather is a step back,
> >> > because we can't possibly learn anything new from that.
> >> 
> >> If Win8 is as broken as we are I'm ok with the module option. It just
> >> sounded to me like right now we don't know of a way to make all machines
> >> somewhat happy, combined with the other pile of random backlight issues
> >> the assumption that we do something (maybe something a bit racy) that
> >> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> >> where the aml is busted for acpi_os=win8, but for the others where this
> >> broke stuff.
> >> 
> >> Or do I miss something here?
> >
> > The ACPI video driver doesn't do anything new now.  It only does things that
> > did work before we started to tell BIOSes we're compatible with Win8.  The
> > reason why they don't work on some machines now is not related to whether or
> > not Win8 is broken, but to what is there in the ACPI tables on those machines.
> > That *is* pure garbage, but Win8 users don't see that (presumably, because
> > Win8 does't execute the AML in question).  We don't see that either with
> > acpi_osi="!Windows 2012" (because then we don't execute that AML either).
> >
> > Whether or not Win8 is broken doesn't matter here.  What matters is that we
> > have to deal with broken AML somehow.
> >
> > One way may be to tell everyone affected by this to pass
> > acpi_osi="!Windows 2012" in the kernel command line or possibly create a
> > blacklist of those machines in the kernel (which Felipe has been pushing for
> > recently) and wash our hands clean of this, but that leaves some exceptionally
> > bad taste in my mouth.
> >
> > The alternative is to try to use native backlight in i915, which we did, but
> > that didn't work on some machines.  I don't think we will know why it didn't
> > work there before we collect some more information and that's not possible
> > without user testing.  So, the idea is to make that testing possible without
> > hacking the kernel and that's why we're introducing the new command line
> > switch.  And we're going to ask users to try it and report back.
> 
> The thing that slightly bugs me with the proposed patches is that
> they're adding a module parameter to i915 to tell ACPI video driver
> whether to quirk the backlight or not. Before you know, we *will* have
> requests to add quirks to i915 to tell ACPI video driver this.
> 
> I think the parameter "Does the ACPI backlight interface work or not"
> belongs to the ACPI video driver.
>
> Feel free to file this in your bikeshedding folder, but I think i915
> should only tell ACPI "I have a native backlight interface".

It kind of does that already through the intel_opregion_init() thing,
so it would be trivial to test intel_opregion_present() in
acpi_video_register().

So yes, we can add a command line option, say 'use_native_backlight' to the
ACPI video driver that will work like this: "if the Intel opregion is present
and video.use_native_backlight is set, skip registering ACPI backlight".

Is that what you want?

Rafael