Date:	Wed, 11 Sep 2013 09:32:11 +0800
From:	Aaron Lu <aaron.lu@...el.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Jani Nikula <jani.nikula@...ux.intel.com>,
	Daniel Vetter <daniel@...ll.ch>, Aaron Lu <aaron.lu@...el.com>,
	ACPI Devel Mailing List <linux-acpi@...r.kernel.org>,
	Matthew Garrett <matthew.garrett@...ula.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
	Yves-Alexis Perez <corsac@...ian.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	Seth Forshee <seth.forshee@...onical.com>,
	"Lee, Chun-Yi" <joeyli.kernel@...il.com>,
	Igor Gnatenko <i.gnatenko.brain@...il.com>,
	Henrique de Moraes Holschuh <hmh@....eng.br>,
	Lee Chun-Yi <jlee@...ell.com>, Len Brown <lenb@...nel.org>
Subject: Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if
 firmware expects Windows 8

On Tue, Sep 10, 2013 at 09:23:04PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
> > On Mon, 09 Sep 2013, "Rafael J. Wysocki" <rjw@...k.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().

It is possible the i915 driver decides not to register a backlight
interface for the graphics card for some reason(memory allocation failed
or it knows the native control does not work on this card or whatever),
so I would prefer let i915 tell ACPI video that it has registered a
native backlight control interface as Jani has said.

Then together with the video.use_native_backlight, we can register or
not register ACPI video backlight interface accordingly. Or rather, we
can simply not register ACPI video backlight interface for Win8 systems
as long as i915 indicates that it has native backlight control(if the
native control is broken, i915 should fix it or blacklist it so that
i915 will not indicate it has native backlight control and ACPI video
will continue to register its own).

How does this sound?

-Aaron

> 
> 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists