| Message ID | 522D89EC.6050103@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote: > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index f466980..75fba17 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > if (INTEL_INFO(dev)->num_pipes) { > /* Must be done after probing outputs */ > intel_opregion_init(dev); > - acpi_video_register(); > + __acpi_video_register(i915_take_over_backlight); > } > > if (IS_GEN5(dev)) I can't compile: DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit declaration of function '__acpi_video_register' [-Werror=implicit-function-declaration] DEBUG: __acpi_video_register(i915_take_over_backlight); DEBUG: ^ DEBUG: cc1: some warnings being treated as errors DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 DEBUG: make[1]: *** [drivers/gpu] Error 2 DEBUG: make: *** [drivers] Error 2
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. Kind regards, Rafael > On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote: > > According to Matthew Garrett, "Windows 8 leaves backlight control up > > to individual graphics drivers rather than making ACPI calls itself. > > There's plenty of evidence to suggest that the Intel driver for > > Windows [8] doesn't use the ACPI interface, including the fact that > > it's broken on a bunch of machines when the OS claims to support > > Windows 8. The simplest thing to do appears to be to disable the > > ACPI backlight interface on these systems". > > > > There's a problem with that approach, however, because simply > > avoiding to register the ACPI backlight interface if the firmware > > calls _OSI for Windows 8 may not work in the following situations: > > (1) The ACPI backlight interface actually works on the given system > > and the i915 driver is not loaded (e.g. another graphics driver > > is used). > > (2) The ACPI backlight interface doesn't work on the given system, > > but there is a vendor platform driver that will register its > > own, equally broken, backlight interface if not prevented from > > doing so by the ACPI subsystem. > > Therefore we need to allow the ACPI backlight interface to be > > registered until the i915 driver is loaded which then will unregister > > it if the firmware has called _OSI for Windows 8 (or will register > > the ACPI video driver without backlight support if not already > > present). > > > > For this reason, introduce an alternative function for registering > > ACPI video, __acpi_video_register(bool), that if ture is passed, > > will check whether or not the ACPI video driver has already been > > registered and whether or not the backlight Windows 8 quirk has to > > be applied. If the quirk has to be applied, it will block the ACPI > > backlight support and either unregister the backlight interface if > > the ACPI video driver has already been registered, or register the > > ACPI video driver without the backlight interface otherwise. Make > > the i915 driver use __acpi_video_register() instead of > > acpi_video_register() in i915_driver_load(), and the param passed > > there is controlled by the i915 module level parameter > > i915_take_over_backlight, which is set to false by default. > > > > This change is evolved from earlier patches of Matthew Garrett, > > Chun-Yi Lee and Seth Forshee and is heavily based on two patches > > from Rafael: > > https://lkml.org/lkml/2013/7/17/720 > > https://lkml.org/lkml/2013/7/24/806 > > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > > --- > > drivers/acpi/internal.h | 2 ++ > > drivers/acpi/video.c | 24 ++++++++++++++++-------- > > drivers/acpi/video_detect.c | 15 ++++++++++++++- > > 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 | 9 +++++++-- > > include/linux/acpi.h | 1 + > > 8 files changed, 47 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > > index 20f4233..a53832e 100644 > > --- a/drivers/acpi/internal.h > > +++ b/drivers/acpi/internal.h > > @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev, > > -------------------------------------------------------------------------- */ > > #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) > > bool acpi_video_backlight_quirks(void); > > +bool acpi_video_verify_backlight_support(void); > > #else > > static inline bool acpi_video_backlight_quirks(void) { return false; } > > +static inline bool acpi_video_verify_backlight_support(void) { return false; } > > #endif > > > > #endif /* _ACPI_INTERNAL_H_ */ > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > > index 5ad5a71..cc709a7 100644 > > --- a/drivers/acpi/video.c > > +++ b/drivers/acpi/video.c > > @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) > > unsigned long long level_current, level_next; > > int result = -EINVAL; > > > > - /* no warning message if acpi_backlight=vendor is used */ > > - if (!acpi_video_backlight_support()) > > + /* no warning message if acpi_backlight=vendor or a quirk is used */ > > + if (!acpi_video_verify_backlight_support()) > > return 0; > > > > if (!device->brightness) > > @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context, > > > > static void acpi_video_dev_register_backlight(struct acpi_video_device *device) > > { > > - if (acpi_video_backlight_support()) { > > + if (acpi_video_verify_backlight_support()) { > > struct backlight_properties props; > > struct pci_dev *pdev; > > acpi_handle acpi_parent; > > @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void) > > return opregion; > > } > > > > -int acpi_video_register(void) > > +int __acpi_video_register(bool backlight_quirks) > > { > > - int result = 0; > > + bool no_backlight; > > + int result; > > + > > + no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false; > > + > > if (register_count) { > > /* > > - * if the function of acpi_video_register is already called, > > - * don't register the acpi_vide_bus again and return no error. > > + * If acpi_video_register() has been called already, don't try > > + * to register acpi_video_bus, but unregister backlight devices > > + * if no backlight support is requested. > > */ > > + if (no_backlight) > > + acpi_video_unregister_backlight(); > > + > > return 0; > > } > > > > @@ -1959,7 +1967,7 @@ int acpi_video_register(void) > > > > return 0; > > } > > -EXPORT_SYMBOL(acpi_video_register); > > +EXPORT_SYMBOL(__acpi_video_register); > > > > void acpi_video_unregister(void) > > { > > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c > > index 940edbf..c7e43ec 100644 > > --- a/drivers/acpi/video_detect.c > > +++ b/drivers/acpi/video_detect.c > > @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void) > > > > bool acpi_video_backlight_quirks(void) > > { > > - return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; > > + if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) { > > + acpi_video_caps_check(); > > + acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT; > > + return true; > > + } > > + return false; > > } > > EXPORT_SYMBOL(acpi_video_backlight_quirks); > > > > @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void) > > } > > EXPORT_SYMBOL(acpi_video_backlight_support); > > > > +/* For the ACPI video driver use only. */ > > +bool acpi_video_verify_backlight_support(void) > > +{ > > + return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ? > > + false : acpi_video_backlight_support(); > > +} > > +EXPORT_SYMBOL(acpi_video_verify_backlight_support); > > + > > /* > > * Use acpi_backlight=vendor/video to force that backlight switching > > * is processed by vendor specific acpi drivers or video.ko driver. > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index f466980..75fba17 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > if (INTEL_INFO(dev)->num_pipes) { > > /* Must be done after probing outputs */ > > intel_opregion_init(dev); > > - acpi_video_register(); > > + __acpi_video_register(i915_take_over_backlight); > > } > > > > if (IS_GEN5(dev)) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 45b3c03..b014398 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/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 = 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; > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 1929bff..b43c430 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly; > > 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); > > diff --git a/include/acpi/video.h b/include/acpi/video.h > > index 0c665b5..dc5b8ad 100644 > > --- a/include/acpi/video.h > > +++ b/include/acpi/video.h > > @@ -17,13 +17,13 @@ struct acpi_device; > > #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200 > > > > #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) > > -extern int acpi_video_register(void); > > +extern int __acpi_video_register(bool backlight_quirks); > > extern void acpi_video_unregister(void); > > extern int acpi_video_unregister_backlight(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(bool backlight_quirks) { return 0; } > > static inline void acpi_video_unregister(void) { return; } > > static inline int acpi_video_unregister_backlight(void) { return; } > > static inline int acpi_video_get_edid(struct acpi_device *device, int type, > > @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, > > } > > #endif > > > > +static inline int acpi_video_register(void) > > +{ > > + return __acpi_video_register(false); > > +} > > + > > #endif > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index a5db4ae..a0dcce5 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid); > > #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO 0x0200 > > #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR 0x0400 > > #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO 0x0800 > > +#define ACPI_VIDEO_SKIP_BACKLIGHT 0x1000 > > > > #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) > > > >
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? Cheers, Daniel
On Mon, 2013-09-09 at 17:21 +0200, Daniel Vetter wrote: > 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. Windows 8 isn't broken because (as far as we can tell) the Intel drivers under Windows 8 never use the ACPI backlight set function. Of course, it would be nice to have that confirmed by Intel.
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. Thanks, Rafael
On 09/09/2013 07:44 PM, Igor Gnatenko wrote: > On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote: >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >> index f466980..75fba17 100644 >> --- a/drivers/gpu/drm/i915/i915_dma.c >> +++ b/drivers/gpu/drm/i915/i915_dma.c >> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) >> if (INTEL_INFO(dev)->num_pipes) { >> /* Must be done after probing outputs */ >> intel_opregion_init(dev); >> - acpi_video_register(); >> + __acpi_video_register(i915_take_over_backlight); >> } >> >> if (IS_GEN5(dev)) > > I can't compile: > > > DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': > DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit > declaration of function > '__acpi_video_register' [-Werror=implicit-function-declaration] > DEBUG: __acpi_video_register(i915_take_over_backlight); > DEBUG: ^ > DEBUG: cc1: some warnings being treated as errors > DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 > DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 > DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 > DEBUG: make[1]: *** [drivers/gpu] Error 2 > DEBUG: make: *** [drivers] Error 2 > The two patches are based on top of Rafael's linux-next tree. I just tried it again, no compile problem for me. I also tried on today Linus' master tree, as there are some updates from i915, two conflicts exist. I've just resolved them and will update it in next revision. If you want to try it now, please use: https://github.com/aaronlu/linux acpi_video_rework Thanks, Aaron
On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote: > On 09/09/2013 07:44 PM, Igor Gnatenko wrote: > > On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote: > >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > >> index f466980..75fba17 100644 > >> --- a/drivers/gpu/drm/i915/i915_dma.c > >> +++ b/drivers/gpu/drm/i915/i915_dma.c > >> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > >> if (INTEL_INFO(dev)->num_pipes) { > >> /* Must be done after probing outputs */ > >> intel_opregion_init(dev); > >> - acpi_video_register(); > >> + __acpi_video_register(i915_take_over_backlight); > >> } > >> > >> if (IS_GEN5(dev)) > > > > I can't compile: > > > > > > DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': > > DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit > > declaration of function > > '__acpi_video_register' [-Werror=implicit-function-declaration] > > DEBUG: __acpi_video_register(i915_take_over_backlight); > > DEBUG: ^ > > DEBUG: cc1: some warnings being treated as errors > > DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 > > DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 > > DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 > > DEBUG: make[1]: *** [drivers/gpu] Error 2 > > DEBUG: make: *** [drivers] Error 2 > > > > The two patches are based on top of Rafael's linux-next tree. I just > tried it again, no compile problem for me. I also tried on today Linus' > master tree, as there are some updates from i915, two conflicts exist. > I've just resolved them and will update it in next revision. > If you want to try it now, please use: > https://github.com/aaronlu/linux acpi_video_rework > > Thanks, > Aaron Thanks. this patch fixes my problems w/ compilation. I've tested this two patches and after apply I have: $ tree /sys/class/backlight/ /sys/class/backlight/ |-- acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 `-- intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight 2 directories, 0 files I think it's didn't unregistered.. I may forget. I need to apply one of patch from Matthew ? Some strings from logs: DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 thinkpad_acpi: Standard ACPI backlight interface available, not loading native one
On 09/10/2013 01:13 PM, Igor Gnatenko wrote: > On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote: >> On 09/09/2013 07:44 PM, Igor Gnatenko wrote: >>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote: >>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >>>> index f466980..75fba17 100644 >>>> --- a/drivers/gpu/drm/i915/i915_dma.c >>>> +++ b/drivers/gpu/drm/i915/i915_dma.c >>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) >>>> if (INTEL_INFO(dev)->num_pipes) { >>>> /* Must be done after probing outputs */ >>>> intel_opregion_init(dev); >>>> - acpi_video_register(); >>>> + __acpi_video_register(i915_take_over_backlight); >>>> } >>>> >>>> if (IS_GEN5(dev)) >>> >>> I can't compile: >>> >>> >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit >>> declaration of function >>> '__acpi_video_register' [-Werror=implicit-function-declaration] >>> DEBUG: __acpi_video_register(i915_take_over_backlight); >>> DEBUG: ^ >>> DEBUG: cc1: some warnings being treated as errors >>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 >>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 >>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 >>> DEBUG: make[1]: *** [drivers/gpu] Error 2 >>> DEBUG: make: *** [drivers] Error 2 >>> >> >> The two patches are based on top of Rafael's linux-next tree. I just >> tried it again, no compile problem for me. I also tried on today Linus' >> master tree, as there are some updates from i915, two conflicts exist. >> I've just resolved them and will update it in next revision. >> If you want to try it now, please use: >> https://github.com/aaronlu/linux acpi_video_rework >> >> Thanks, >> Aaron > > Thanks. this patch fixes my problems w/ compilation. I've tested this > two patches and after apply I have: > $ tree /sys/class/backlight/ > /sys/class/backlight/ > |-- acpi_video0 > -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 > `-- intel_backlight > -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight > > 2 directories, 0 files > > I think it's didn't unregistered.. I may forget. I need to apply one of > patch from Matthew ? You need to specify i915.take_over_backlight=1 in kernel cmdline, that module option is set to false by default for now. Thanks for the test. -Aaron > > Some strings from logs: > DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 > thinkpad_acpi: Standard ACPI backlight interface available, not loading > native one >
On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote: > On 09/10/2013 01:13 PM, Igor Gnatenko wrote: > > On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote: > >> On 09/09/2013 07:44 PM, Igor Gnatenko wrote: > >>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote: > >>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > >>>> index f466980..75fba17 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_dma.c > >>>> +++ b/drivers/gpu/drm/i915/i915_dma.c > >>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > >>>> if (INTEL_INFO(dev)->num_pipes) { > >>>> /* Must be done after probing outputs */ > >>>> intel_opregion_init(dev); > >>>> - acpi_video_register(); > >>>> + __acpi_video_register(i915_take_over_backlight); > >>>> } > >>>> > >>>> if (IS_GEN5(dev)) > >>> > >>> I can't compile: > >>> > >>> > >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': > >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit > >>> declaration of function > >>> '__acpi_video_register' [-Werror=implicit-function-declaration] > >>> DEBUG: __acpi_video_register(i915_take_over_backlight); > >>> DEBUG: ^ > >>> DEBUG: cc1: some warnings being treated as errors > >>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 > >>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 > >>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 > >>> DEBUG: make[1]: *** [drivers/gpu] Error 2 > >>> DEBUG: make: *** [drivers] Error 2 > >>> > >> > >> The two patches are based on top of Rafael's linux-next tree. I just > >> tried it again, no compile problem for me. I also tried on today Linus' > >> master tree, as there are some updates from i915, two conflicts exist. > >> I've just resolved them and will update it in next revision. > >> If you want to try it now, please use: > >> https://github.com/aaronlu/linux acpi_video_rework > >> > >> Thanks, > >> Aaron > > > > Thanks. this patch fixes my problems w/ compilation. I've tested this > > two patches and after apply I have: > > $ tree /sys/class/backlight/ > > /sys/class/backlight/ > > |-- acpi_video0 > > -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 > > `-- intel_backlight > > -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight > > > > 2 directories, 0 files > > > > I think it's didn't unregistered.. I may forget. I need to apply one of > > patch from Matthew ? > > You need to specify i915.take_over_backlight=1 in kernel cmdline, that > module option is set to false by default for now. > > Thanks for the test. > > -Aaron > > > > > Some strings from logs: > > DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 > > thinkpad_acpi: Standard ACPI backlight interface available, not loading > > native one > > > Thanks for quick answer. Yes. This option do unregister. Thanks. but for this patch-set I also need "[PATCH 2/3] ACPI / video: Always call acpi_video_init_brightness() on init" from Matthew (for notifications in DE).
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote: > According to Matthew Garrett, "Windows 8 leaves backlight control up > to individual graphics drivers rather than making ACPI calls itself. > There's plenty of evidence to suggest that the Intel driver for > Windows [8] doesn't use the ACPI interface, including the fact that > it's broken on a bunch of machines when the OS claims to support > Windows 8. The simplest thing to do appears to be to disable the > ACPI backlight interface on these systems". > > There's a problem with that approach, however, because simply > avoiding to register the ACPI backlight interface if the firmware > calls _OSI for Windows 8 may not work in the following situations: > (1) The ACPI backlight interface actually works on the given system > and the i915 driver is not loaded (e.g. another graphics driver > is used). > (2) The ACPI backlight interface doesn't work on the given system, > but there is a vendor platform driver that will register its > own, equally broken, backlight interface if not prevented from > doing so by the ACPI subsystem. > Therefore we need to allow the ACPI backlight interface to be > registered until the i915 driver is loaded which then will unregister > it if the firmware has called _OSI for Windows 8 (or will register > the ACPI video driver without backlight support if not already > present). > > For this reason, introduce an alternative function for registering > ACPI video, __acpi_video_register(bool), that if ture is passed, > will check whether or not the ACPI video driver has already been > registered and whether or not the backlight Windows 8 quirk has to > be applied. If the quirk has to be applied, it will block the ACPI > backlight support and either unregister the backlight interface if > the ACPI video driver has already been registered, or register the > ACPI video driver without the backlight interface otherwise. Make > the i915 driver use __acpi_video_register() instead of > acpi_video_register() in i915_driver_load(), and the param passed > there is controlled by the i915 module level parameter > i915_take_over_backlight, which is set to false by default. > > This change is evolved from earlier patches of Matthew Garrett, > Chun-Yi Lee and Seth Forshee and is heavily based on two patches > from Rafael: > https://lkml.org/lkml/2013/7/17/720 > https://lkml.org/lkml/2013/7/24/806 > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com> > --- > drivers/acpi/internal.h | 2 ++ > drivers/acpi/video.c | 24 ++++++++++++++++-------- > drivers/acpi/video_detect.c | 15 ++++++++++++++- > 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 | 9 +++++++-- > include/linux/acpi.h | 1 + > 8 files changed, 47 insertions(+), 12 deletions(-)
On 09/10/2013 01:22 PM, Igor Gnatenko wrote: > On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote: >> On 09/10/2013 01:13 PM, Igor Gnatenko wrote: >>> On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote: >>>> On 09/09/2013 07:44 PM, Igor Gnatenko wrote: >>>>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote: >>>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >>>>>> index f466980..75fba17 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_dma.c >>>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c >>>>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) >>>>>> if (INTEL_INFO(dev)->num_pipes) { >>>>>> /* Must be done after probing outputs */ >>>>>> intel_opregion_init(dev); >>>>>> - acpi_video_register(); >>>>>> + __acpi_video_register(i915_take_over_backlight); >>>>>> } >>>>>> >>>>>> if (IS_GEN5(dev)) >>>>> >>>>> I can't compile: >>>>> >>>>> >>>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': >>>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit >>>>> declaration of function >>>>> '__acpi_video_register' [-Werror=implicit-function-declaration] >>>>> DEBUG: __acpi_video_register(i915_take_over_backlight); >>>>> DEBUG: ^ >>>>> DEBUG: cc1: some warnings being treated as errors >>>>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 >>>>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 >>>>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 >>>>> DEBUG: make[1]: *** [drivers/gpu] Error 2 >>>>> DEBUG: make: *** [drivers] Error 2 >>>>> >>>> >>>> The two patches are based on top of Rafael's linux-next tree. I just >>>> tried it again, no compile problem for me. I also tried on today Linus' >>>> master tree, as there are some updates from i915, two conflicts exist. >>>> I've just resolved them and will update it in next revision. >>>> If you want to try it now, please use: >>>> https://github.com/aaronlu/linux acpi_video_rework >>>> >>>> Thanks, >>>> Aaron >>> >>> Thanks. this patch fixes my problems w/ compilation. I've tested this >>> two patches and after apply I have: >>> $ tree /sys/class/backlight/ >>> /sys/class/backlight/ >>> |-- acpi_video0 >>> -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 >>> `-- intel_backlight >>> -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight >>> >>> 2 directories, 0 files >>> >>> I think it's didn't unregistered.. I may forget. I need to apply one of >>> patch from Matthew ? >> >> You need to specify i915.take_over_backlight=1 in kernel cmdline, that >> module option is set to false by default for now. >> >> Thanks for the test. >> >> -Aaron >> >>> >>> Some strings from logs: >>> DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 >>> thinkpad_acpi: Standard ACPI backlight interface available, not loading >>> native one >>> >> > > Thanks for quick answer. Yes. This option do unregister. Thanks. but for > this patch-set I also need "[PATCH 2/3] ACPI / video: Always call > acpi_video_init_brightness() on init" from Matthew (for notifications in > DE). That patch is reverted as it cause problem for other system: https://bugs.freedesktop.org/show_bug.cgi?id=68355 OTOH, the thinkpad-acpi module already has a call to _BCL except that the tpacpi_acpi_handle_locate failed to locate video controller's handle: https://bugzilla.kernel.org/show_bug.cgi?id=51231#c121 I'll see if I can figure out why. Thanks, Aaron
On Mon, Sep 09, 2013 at 11:32:10AM +0200, 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? I've grown _very_ Sorry, no general rule has been found. As Rafael has said, it appears to be random... And in addition to the "shipped with Win7 with a Win8 upgrade option" case, I also find a Sony laptop that has Win8 pre-installed and a broken ACPI video backlight interface. Its ACPI video backlight control method is simply a stub, so even the acpi_osi="!Windows 2012" will not provide a working backlight for this system. -Aaron > 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 ... > > Cheers, Daniel > > On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote: > > According to Matthew Garrett, "Windows 8 leaves backlight control up > > to individual graphics drivers rather than making ACPI calls itself. > > There's plenty of evidence to suggest that the Intel driver for > > Windows [8] doesn't use the ACPI interface, including the fact that > > it's broken on a bunch of machines when the OS claims to support > > Windows 8. The simplest thing to do appears to be to disable the > > ACPI backlight interface on these systems". > > > > There's a problem with that approach, however, because simply > > avoiding to register the ACPI backlight interface if the firmware > > calls _OSI for Windows 8 may not work in the following situations: > > (1) The ACPI backlight interface actually works on the given system > > and the i915 driver is not loaded (e.g. another graphics driver > > is used). > > (2) The ACPI backlight interface doesn't work on the given system, > > but there is a vendor platform driver that will register its > > own, equally broken, backlight interface if not prevented from > > doing so by the ACPI subsystem. > > Therefore we need to allow the ACPI backlight interface to be > > registered until the i915 driver is loaded which then will unregister > > it if the firmware has called _OSI for Windows 8 (or will register > > the ACPI video driver without backlight support if not already > > present). > > > > For this reason, introduce an alternative function for registering > > ACPI video, __acpi_video_register(bool), that if ture is passed, > > will check whether or not the ACPI video driver has already been > > registered and whether or not the backlight Windows 8 quirk has to > > be applied. If the quirk has to be applied, it will block the ACPI > > backlight support and either unregister the backlight interface if > > the ACPI video driver has already been registered, or register the > > ACPI video driver without the backlight interface otherwise. Make > > the i915 driver use __acpi_video_register() instead of > > acpi_video_register() in i915_driver_load(), and the param passed > > there is controlled by the i915 module level parameter > > i915_take_over_backlight, which is set to false by default. > > > > This change is evolved from earlier patches of Matthew Garrett, > > Chun-Yi Lee and Seth Forshee and is heavily based on two patches > > from Rafael: > > https://lkml.org/lkml/2013/7/17/720 > > https://lkml.org/lkml/2013/7/24/806 > > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > > --- > > drivers/acpi/internal.h | 2 ++ > > drivers/acpi/video.c | 24 ++++++++++++++++-------- > > drivers/acpi/video_detect.c | 15 ++++++++++++++- > > 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 | 9 +++++++-- > > include/linux/acpi.h | 1 + > > 8 files changed, 47 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > > index 20f4233..a53832e 100644 > > --- a/drivers/acpi/internal.h > > +++ b/drivers/acpi/internal.h > > @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev, > > -------------------------------------------------------------------------- */ > > #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) > > bool acpi_video_backlight_quirks(void); > > +bool acpi_video_verify_backlight_support(void); > > #else > > static inline bool acpi_video_backlight_quirks(void) { return false; } > > +static inline bool acpi_video_verify_backlight_support(void) { return false; } > > #endif > > > > #endif /* _ACPI_INTERNAL_H_ */ > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > > index 5ad5a71..cc709a7 100644 > > --- a/drivers/acpi/video.c > > +++ b/drivers/acpi/video.c > > @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) > > unsigned long long level_current, level_next; > > int result = -EINVAL; > > > > - /* no warning message if acpi_backlight=vendor is used */ > > - if (!acpi_video_backlight_support()) > > + /* no warning message if acpi_backlight=vendor or a quirk is used */ > > + if (!acpi_video_verify_backlight_support()) > > return 0; > > > > if (!device->brightness) > > @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context, > > > > static void acpi_video_dev_register_backlight(struct acpi_video_device *device) > > { > > - if (acpi_video_backlight_support()) { > > + if (acpi_video_verify_backlight_support()) { > > struct backlight_properties props; > > struct pci_dev *pdev; > > acpi_handle acpi_parent; > > @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void) > > return opregion; > > } > > > > -int acpi_video_register(void) > > +int __acpi_video_register(bool backlight_quirks) > > { > > - int result = 0; > > + bool no_backlight; > > + int result; > > + > > + no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false; > > + > > if (register_count) { > > /* > > - * if the function of acpi_video_register is already called, > > - * don't register the acpi_vide_bus again and return no error. > > + * If acpi_video_register() has been called already, don't try > > + * to register acpi_video_bus, but unregister backlight devices > > + * if no backlight support is requested. > > */ > > + if (no_backlight) > > + acpi_video_unregister_backlight(); > > + > > return 0; > > } > > > > @@ -1959,7 +1967,7 @@ int acpi_video_register(void) > > > > return 0; > > } > > -EXPORT_SYMBOL(acpi_video_register); > > +EXPORT_SYMBOL(__acpi_video_register); > > > > void acpi_video_unregister(void) > > { > > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c > > index 940edbf..c7e43ec 100644 > > --- a/drivers/acpi/video_detect.c > > +++ b/drivers/acpi/video_detect.c > > @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void) > > > > bool acpi_video_backlight_quirks(void) > > { > > - return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; > > + if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) { > > + acpi_video_caps_check(); > > + acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT; > > + return true; > > + } > > + return false; > > } > > EXPORT_SYMBOL(acpi_video_backlight_quirks); > > > > @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void) > > } > > EXPORT_SYMBOL(acpi_video_backlight_support); > > > > +/* For the ACPI video driver use only. */ > > +bool acpi_video_verify_backlight_support(void) > > +{ > > + return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ? > > + false : acpi_video_backlight_support(); > > +} > > +EXPORT_SYMBOL(acpi_video_verify_backlight_support); > > + > > /* > > * Use acpi_backlight=vendor/video to force that backlight switching > > * is processed by vendor specific acpi drivers or video.ko driver. > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index f466980..75fba17 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > if (INTEL_INFO(dev)->num_pipes) { > > /* Must be done after probing outputs */ > > intel_opregion_init(dev); > > - acpi_video_register(); > > + __acpi_video_register(i915_take_over_backlight); > > } > > > > if (IS_GEN5(dev)) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 45b3c03..b014398 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/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 = 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; > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 1929bff..b43c430 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly; > > 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); > > diff --git a/include/acpi/video.h b/include/acpi/video.h > > index 0c665b5..dc5b8ad 100644 > > --- a/include/acpi/video.h > > +++ b/include/acpi/video.h > > @@ -17,13 +17,13 @@ struct acpi_device; > > #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200 > > > > #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) > > -extern int acpi_video_register(void); > > +extern int __acpi_video_register(bool backlight_quirks); > > extern void acpi_video_unregister(void); > > extern int acpi_video_unregister_backlight(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(bool backlight_quirks) { return 0; } > > static inline void acpi_video_unregister(void) { return; } > > static inline int acpi_video_unregister_backlight(void) { return; } > > static inline int acpi_video_get_edid(struct acpi_device *device, int type, > > @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, > > } > > #endif > > > > +static inline int acpi_video_register(void) > > +{ > > + return __acpi_video_register(false); > > +} > > + > > #endif > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index a5db4ae..a0dcce5 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid); > > #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO 0x0200 > > #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR 0x0400 > > #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO 0x0800 > > +#define ACPI_VIDEO_SKIP_BACKLIGHT 0x1000 > > > > #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) > > > > -- > > 1.8.4.12.g2ea3df6 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > -- > 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
On Mon, 09 Sep 2013, "Rafael J. Wysocki" <rjw@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". What ACPI video driver does with that information is its business. The desired thing to do here would be to check if there's a module parameter or a quirk to disable the ACPI backlight interface. acpi_backlight=DTRT? ;) BR, Jani.
On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote: > I think the parameter "Does the ACPI backlight interface work or not" > belongs to the ACPI video driver. That depends on how Windows 8 works. If Windows 8 policy is handled by the GPU drivers then it belongs in i915. If it's handled by the ACPI code then it belongs in the ACPI code. But I have no way of determining that, whereas you work for a company that produces a Windows 8 video driver… -- Matthew Garrett <matthew.garrett@nebula.com>
On Tue, 2013-09-10 at 17:21 +0300, Jani Nikula wrote: > On Tue, 10 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote: > > On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote: > > > >> I think the parameter "Does the ACPI backlight interface work or not" > >> belongs to the ACPI video driver. > > > > That depends on how Windows 8 works. If Windows 8 policy is handled by > > the GPU drivers then it belongs in i915. If it's handled by the ACPI > > code then it belongs in the ACPI code. > > I fail to see the logic. Windows 8 policy dictates whether we can use > the AML code or not. IMHO, ACPI code is in the best position to figure > this out and quirk as necessary. It's the part that knows about Windows > 8, not i915. So if nvidia hardware uses the ACPI interface and Intel doesn't, we should still quirk it in the ACPI driver?
On Tue, 10 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote: > On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote: > >> I think the parameter "Does the ACPI backlight interface work or not" >> belongs to the ACPI video driver. > > That depends on how Windows 8 works. If Windows 8 policy is handled by > the GPU drivers then it belongs in i915. If it's handled by the ACPI > code then it belongs in the ACPI code. I fail to see the logic. Windows 8 policy dictates whether we can use the AML code or not. IMHO, ACPI code is in the best position to figure this out and quirk as necessary. It's the part that knows about Windows 8, not i915. > But I have no way of determining that, whereas you work for a company > that produces a Windows 8 video driver… Here I do see the logic. However see signature; I'm not in the best of positions to figure out things about Windows 8 video drivers. ;) But I'll see what I can do. BR, Jani.
On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote: > On Mon, 09 Sep 2013, "Rafael J. Wysocki" <rjw@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
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@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(). 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Sep 2013, Aaron Lu <aaron.lu@intel.com> wrote: > 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? Sounds good to me. Before plunging forward, have you observed any difference between the boot modes? We have reports [1] that the backlight behaviour is different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole story. Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code paths, what guarantees do we have of UEFI+CSM or legacy boots working? BR, Jani. [1] https://bugzilla.kernel.org/show_bug.cgi?id=47941#c96
On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote: > Before plunging forward, have you observed any difference between the > boot modes? We have reports [1] that the backlight behaviour is > different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the > acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole > story. > > Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code > paths, what guarantees do we have of UEFI+CSM or legacy boots working? We have no evidence of Windows behaving differently based on the exposed firmware type.
On mer., 2013-09-11 at 08:45 +0000, Matthew Garrett wrote: > On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote: > > > Before plunging forward, have you observed any difference between the > > boot modes? We have reports [1] that the backlight behaviour is > > different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the > > acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole > > story. > > > > Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code > > paths, what guarantees do we have of UEFI+CSM or legacy boots working? > > We have no evidence of Windows behaving differently based on the exposed > firmware type. > I do have an X230 which boots fine using pure UEFI or UEFI+CSM (and I guess I can try booting it with a grml for pure legacy), so I can do tests if needed. Regards,
On Wed, 11 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote: > On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote: > >> Before plunging forward, have you observed any difference between the >> boot modes? We have reports [1] that the backlight behaviour is >> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the >> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole >> story. >> >> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code >> paths, what guarantees do we have of UEFI+CSM or legacy boots working? > > We have no evidence of Windows behaving differently based on the exposed > firmware type. By "behaving differently", do you mean internally adapting to the boot mode, or exhibiting different behaviour to the user? We have evidence of the firmware behaving differently (VBT, backlight) based on the boot mode, all else being equal. We don't adapt to that, and we fail. I don't know if we should adapt, or do things differently altogether. I don't even know if Windows 8 works on all boot modes on the machines in question. BR, Jani.
On Wed, 2013-09-11 at 13:29 +0300, Jani Nikula wrote: > On Wed, 11 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote: > > On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote: > > > >> Before plunging forward, have you observed any difference between the > >> boot modes? We have reports [1] that the backlight behaviour is > >> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the > >> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole > >> story. > >> > >> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code > >> paths, what guarantees do we have of UEFI+CSM or legacy boots working? > > > > We have no evidence of Windows behaving differently based on the exposed > > firmware type. > > By "behaving differently", do you mean internally adapting to the boot > mode, or exhibiting different behaviour to the user? As far as backlight control goes, both. > We have evidence of the firmware behaving differently (VBT, backlight) > based on the boot mode, all else being equal. We don't adapt to that, > and we fail. I don't know if we should adapt, or do things differently > altogether. I don't even know if Windows 8 works on all boot modes on > the machines in question. Sure, but Windows knows nothing about VBT or opregion-backed backlight control. That's up to the Intel driver.
On Wed, Sep 11, 2013 at 11:45:19AM +0300, Jani Nikula wrote: > On Wed, 11 Sep 2013, Aaron Lu <aaron.lu@intel.com> wrote: > > 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? > > Sounds good to me. > > Before plunging forward, have you observed any difference between the > boot modes? We have reports [1] that the backlight behaviour is Not yet from ACPI's point of view. > different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the > acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole > story. This check in patch 2/2 is a policy: for Win8 system, we think the native backlight control has a better chance of working than the ACPI video's, so I think the check is enough in ACPI video. > > Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code > paths, what guarantees do we have of UEFI+CSM or legacy boots working? I suppose the 'tested BIOS code paths' means the pure UEFI boot mode? I don't know what guatantees do we have since I don't know what happened underneath after the backlight register is set in i915 driver, you or other i915 driver people should know more than I do :-) BTW, after the backlight register is set in i915, is it that some find of firmware code will run in response to the setting of the register(e.g. the BLC_PWM_CTL/BLC_PWM_CPU_CTL/PCI_LBPC reg)? Thanks, Aaron
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..a53832e 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev, -------------------------------------------------------------------------- */ #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) bool acpi_video_backlight_quirks(void); +bool acpi_video_verify_backlight_support(void); #else static inline bool acpi_video_backlight_quirks(void) { return false; } +static inline bool acpi_video_verify_backlight_support(void) { return false; } #endif #endif /* _ACPI_INTERNAL_H_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 5ad5a71..cc709a7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) unsigned long long level_current, level_next; int result = -EINVAL; - /* no warning message if acpi_backlight=vendor is used */ - if (!acpi_video_backlight_support()) + /* no warning message if acpi_backlight=vendor or a quirk is used */ + if (!acpi_video_verify_backlight_support()) return 0; if (!device->brightness) @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context, static void acpi_video_dev_register_backlight(struct acpi_video_device *device) { - if (acpi_video_backlight_support()) { + if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; acpi_handle acpi_parent; @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void) return opregion; } -int acpi_video_register(void) +int __acpi_video_register(bool backlight_quirks) { - int result = 0; + bool no_backlight; + int result; + + no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false; + if (register_count) { /* - * if the function of acpi_video_register is already called, - * don't register the acpi_vide_bus again and return no error. + * If acpi_video_register() has been called already, don't try + * to register acpi_video_bus, but unregister backlight devices + * if no backlight support is requested. */ + if (no_backlight) + acpi_video_unregister_backlight(); + return 0; } @@ -1959,7 +1967,7 @@ int acpi_video_register(void) return 0; } -EXPORT_SYMBOL(acpi_video_register); +EXPORT_SYMBOL(__acpi_video_register); void acpi_video_unregister(void) { diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..c7e43ec 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void) bool acpi_video_backlight_quirks(void) { - return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; + if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) { + acpi_video_caps_check(); + acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT; + return true; + } + return false; } EXPORT_SYMBOL(acpi_video_backlight_quirks); @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void) } EXPORT_SYMBOL(acpi_video_backlight_support); +/* For the ACPI video driver use only. */ +bool acpi_video_verify_backlight_support(void) +{ + return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ? + false : acpi_video_backlight_support(); +} +EXPORT_SYMBOL(acpi_video_verify_backlight_support); + /* * Use acpi_backlight=vendor/video to force that backlight switching * is processed by vendor specific acpi drivers or video.ko driver. diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev); - acpi_video_register(); + __acpi_video_register(i915_take_over_backlight); } if (IS_GEN5(dev)) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 45b3c03..b014398 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/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 = 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; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1929bff..b43c430 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly; 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); diff --git a/include/acpi/video.h b/include/acpi/video.h index 0c665b5..dc5b8ad 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -17,13 +17,13 @@ struct acpi_device; #define ACPI_VIDEO_DISPLAY_LEGACY_TV 0x0200 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) -extern int acpi_video_register(void); +extern int __acpi_video_register(bool backlight_quirks); extern void acpi_video_unregister(void); extern int acpi_video_unregister_backlight(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(bool backlight_quirks) { return 0; } static inline void acpi_video_unregister(void) { return; } static inline int acpi_video_unregister_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, } #endif +static inline int acpi_video_register(void) +{ + return __acpi_video_register(false); +} + #endif diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a5db4ae..a0dcce5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid); #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO 0x0200 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR 0x0400 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO 0x0800 +#define ACPI_VIDEO_SKIP_BACKLIGHT 0x1000 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
According to Matthew Garrett, "Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows [8] doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems". There's a problem with that approach, however, because simply avoiding to register the ACPI backlight interface if the firmware calls _OSI for Windows 8 may not work in the following situations: (1) The ACPI backlight interface actually works on the given system and the i915 driver is not loaded (e.g. another graphics driver is used). (2) The ACPI backlight interface doesn't work on the given system, but there is a vendor platform driver that will register its own, equally broken, backlight interface if not prevented from doing so by the ACPI subsystem. Therefore we need to allow the ACPI backlight interface to be registered until the i915 driver is loaded which then will unregister it if the firmware has called _OSI for Windows 8 (or will register the ACPI video driver without backlight support if not already present). For this reason, introduce an alternative function for registering ACPI video, __acpi_video_register(bool), that if ture is passed, will check whether or not the ACPI video driver has already been registered and whether or not the backlight Windows 8 quirk has to be applied. If the quirk has to be applied, it will block the ACPI backlight support and either unregister the backlight interface if the ACPI video driver has already been registered, or register the ACPI video driver without the backlight interface otherwise. Make the i915 driver use __acpi_video_register() instead of acpi_video_register() in i915_driver_load(), and the param passed there is controlled by the i915 module level parameter i915_take_over_backlight, which is set to false by default. This change is evolved from earlier patches of Matthew Garrett, Chun-Yi Lee and Seth Forshee and is heavily based on two patches from Rafael: https://lkml.org/lkml/2013/7/17/720 https://lkml.org/lkml/2013/7/24/806 Signed-off-by: Aaron Lu <aaron.lu@intel.com> --- drivers/acpi/internal.h | 2 ++ drivers/acpi/video.c | 24 ++++++++++++++++-------- drivers/acpi/video_detect.c | 15 ++++++++++++++- 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 | 9 +++++++-- include/linux/acpi.h | 1 + 8 files changed, 47 insertions(+), 12 deletions(-)