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