On 07/15/2013 07:42 PM, Rafael J. Wysocki wrote: > On Monday, July 15, 2013 10:36:15 AM Aaron Lu wrote: >> On 07/13/2013 08:46 AM, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki at intel.com> >>> >>> 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_with_quirks(), that 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_with_quirks() instead of >>> acpi_video_register() in i915_driver_load(). >>> >>> This change is based on earlier patches from Matthew Garrett, >>> Chun-Yi Lee and Seth Forshee and Aaron Lu's comments. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com> >> >> Reviewed-by: Aaron Lu <aaron.lu at intel.com> >> >> BTW, I also tested on a Toshiba laptop Z830 where its AML code >> claims support of win8, the result is as expected: ACPI video >> interface is removed, i915 Xorg driver picks intel_backlight. >> >> Thanks for the fix. > > Cool, thanks for testing this! > > Can you please also ask bug reporters in the BZ entires related to this to test > it too? Sure. To be clear, I actually tested the patch in your linux-next branch, which turned out to be a little different in that you have fixed the problem Igor raised here. I'll ask reporters to test on a stable tree with the following two patches on top: https://patchwork.kernel.org/patch/2812951/ (expose OSI version) https://patchwork.kernel.org/patch/2827793/ (your updated patch) or your linux-next branch, whichever they like. -Aaron > > Rafael > > >>> --- >>> drivers/acpi/internal.h | 11 ++++++ >>> drivers/acpi/video.c | 65 ++++++++++++++++++++++++++++++++++++---- >>> drivers/acpi/video_detect.c | 21 ++++++++++++ >>> drivers/gpu/drm/i915/i915_dma.c | 2 - >>> include/acpi/video.h | 11 ++++++ >>> include/linux/acpi.h | 1 >>> 6 files changed, 103 insertions(+), 8 deletions(-) >>> >>> Index: linux-pm/drivers/acpi/video.c >>> =================================================================== >>> --- linux-pm.orig/drivers/acpi/video.c >>> +++ linux-pm/drivers/acpi/video.c >>> @@ -44,6 +44,8 @@ >>> #include <linux/suspend.h> >>> #include <acpi/video.h> >>> >>> +#include "internal.h" >>> + >>> #define PREFIX "ACPI: " >>> >>> #define ACPI_VIDEO_BUS_NAME "Video Bus" >>> @@ -898,7 +900,7 @@ static void acpi_video_device_find_cap(s >>> device->cap._DDC = 1; >>> } >>> >>> - if (acpi_video_backlight_support()) { >>> + if (acpi_video_verify_backlight_support()) { >>> struct backlight_properties props; >>> struct pci_dev *pdev; >>> acpi_handle acpi_parent; >>> @@ -1854,6 +1856,46 @@ static int acpi_video_bus_remove(struct >>> return 0; >>> } >>> >>> +static acpi_status video_unregister_backlight(acpi_handle handle, u32 lvl, >>> + void *context, void **rv) >>> +{ >>> + struct acpi_device *acpi_dev; >>> + struct acpi_video_bus *video; >>> + struct acpi_video_device *dev, *next; >>> + >>> + if (acpi_bus_get_device(handle, &acpi_dev)) >>> + return AE_OK; >>> + >>> + if (acpi_match_device_ids(acpi_dev, video_device_ids)) >>> + return AE_OK; >>> + >>> + video = acpi_driver_data(acpi_dev); >>> + if (!video) >>> + return AE_OK; >>> + >>> + acpi_video_bus_stop_devices(video); >>> + mutex_lock(&video->device_list_lock); >>> + list_for_each_entry_safe(dev, next, &video->video_device_list, entry) { >>> + if (dev->backlight) { >>> + backlight_device_unregister(dev->backlight); >>> + dev->backlight = NULL; >>> + kfree(dev->brightness->levels); >>> + kfree(dev->brightness); >>> + } >>> + if (dev->cooling_dev) { >>> + sysfs_remove_link(&dev->dev->dev.kobj, >>> + "thermal_cooling"); >>> + sysfs_remove_link(&dev->cooling_dev->device.kobj, >>> + "device"); >>> + thermal_cooling_device_unregister(dev->cooling_dev); >>> + dev->cooling_dev = NULL; >>> + } >>> + } >>> + mutex_unlock(&video->device_list_lock); >>> + acpi_video_bus_start_devices(video); >>> + return AE_OK; >>> +} >>> + >>> static int __init is_i740(struct pci_dev *dev) >>> { >>> if (dev->device == 0x00D1) >>> @@ -1885,14 +1927,25 @@ static int __init intel_opregion_present >>> 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_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, >>> + ACPI_UINT32_MAX, >>> + video_unregister_backlight, >>> + NULL, NULL, NULL); >>> + >>> return 0; >>> } >>> >>> @@ -1908,7 +1961,7 @@ int acpi_video_register(void) >>> >>> return 0; >>> } >>> -EXPORT_SYMBOL(acpi_video_register); >>> +EXPORT_SYMBOL(__acpi_video_register); >>> >>> void acpi_video_unregister(void) >>> { >>> Index: linux-pm/drivers/gpu/drm/i915/i915_dma.c >>> =================================================================== >>> --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c >>> +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c >>> @@ -1660,7 +1660,7 @@ int i915_driver_load(struct drm_device * >>> if (INTEL_INFO(dev)->num_pipes) { >>> /* Must be done after probing outputs */ >>> intel_opregion_init(dev); >>> - acpi_video_register(); >>> + acpi_video_register_with_quirks(); >>> } >>> >>> if (IS_GEN5(dev)) >>> Index: linux-pm/include/acpi/video.h >>> =================================================================== >>> --- linux-pm.orig/include/acpi/video.h >>> +++ linux-pm/include/acpi/video.h >>> @@ -17,12 +17,21 @@ 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); >>> +static inline int acpi_video_register(void) >>> +{ >>> + return __acpi_video_register(false); >>> +} >>> +static inline int acpi_video_register_with_quirks(void) >>> +{ >>> + return __acpi_video_register(true); >>> +} >>> extern void acpi_video_unregister(void); >>> extern int acpi_video_get_edid(struct acpi_device *device, int type, >>> int device_id, void **edid); >>> #else >>> static inline int acpi_video_register(void) { return 0; } >>> +static inline int acpi_video_register_with_quirks(void) { return 0; } >>> static inline void acpi_video_unregister(void) { return; } >>> static inline int acpi_video_get_edid(struct acpi_device *device, int type, >>> int device_id, void **edid) >>> Index: linux-pm/drivers/acpi/video_detect.c >>> =================================================================== >>> --- linux-pm.orig/drivers/acpi/video_detect.c >>> +++ linux-pm/drivers/acpi/video_detect.c >>> @@ -38,6 +38,8 @@ >>> #include <linux/dmi.h> >>> #include <linux/pci.h> >>> >>> +#include "internal.h" >>> + >>> #define PREFIX "ACPI: " >>> >>> ACPI_MODULE_NAME("video"); >>> @@ -234,6 +236,17 @@ static void acpi_video_caps_check(void) >>> acpi_video_get_capabilities(NULL); >>> } >>> >>> +bool acpi_video_backlight_quirks(void) >>> +{ >>> + 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); >>> + >>> /* Promote the vendor interface instead of the generic video module. >>> * This function allow DMI blacklists to be implemented by externals >>> * platform drivers instead of putting a big blacklist in video_detect.c >>> @@ -278,6 +291,14 @@ int acpi_video_backlight_support(void) >>> } >>> EXPORT_SYMBOL(acpi_video_backlight_support); >>> >>> +/* For the ACPI video driver's 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. >>> Index: linux-pm/include/linux/acpi.h >>> =================================================================== >>> --- linux-pm.orig/include/linux/acpi.h >>> +++ linux-pm/include/linux/acpi.h >>> @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *gui >>> #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) >>> >>> Index: linux-pm/drivers/acpi/internal.h >>> =================================================================== >>> --- linux-pm.orig/drivers/acpi/internal.h >>> +++ linux-pm/drivers/acpi/internal.h >>> @@ -164,4 +164,15 @@ struct platform_device; >>> int acpi_create_platform_device(struct acpi_device *adev, >>> const struct acpi_device_id *id); >>> >>> +/*-------------------------------------------------------------------------- >>> + Video >>> + -------------------------------------------------------------------------- */ >>> +#ifdef CONFIG_ACPI_VIDEO >>> +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_ */ >>> >>
