Windows 8 introduced new policy for backlight control by pushing it out to
graphics drivers. This appears to have coincided with a range of vendors
adding Windows 8 checks to their backlight control code which trigger either
awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
thing to do would be to just disable ACPI backlight control entirely if the
firmware indicates Windows 8 support, but it's entirely possible that
individual graphics drivers might still make use of the ACPI functionality in
preference to native control.
The first two patches in this series are picked from other patchesets aimed at
solving similar problems. The last simply unregisters ACPI backlight control
on Windows 8 systems when using an Intel GPU. Similar code could be added to
other drivers, but I'm reluctant to do so without further investigation as
to the behaviour of the vendor drivers under Windows.
--
Matthew Garrett | [email protected]
Drivers may need to make policy decisions based on the OS that the firmware
believes it's interacting with. ACPI firmware will make a series of _OSI
calls, starting from the oldest OS version they support and ending with the
most recent. Add a function to return the last successful call so that
drivers know what the firmware's expecting.
Based on a patch by Seth Forshee <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
Cc: Seth Forshee <[email protected]>
---
drivers/acpi/acpica/aclocal.h | 13 -------------
drivers/acpi/acpica/utxface.c | 19 +++++++++++++++++++
include/acpi/acpixf.h | 22 ++++++++++++++++++++++
3 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index d5bfbd3..8a2f532 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -948,19 +948,6 @@ struct acpi_bit_register_info {
/* Structs and definitions for _OSI support and I/O port validation */
-#define ACPI_OSI_WIN_2000 0x01
-#define ACPI_OSI_WIN_XP 0x02
-#define ACPI_OSI_WIN_XP_SP1 0x03
-#define ACPI_OSI_WINSRV_2003 0x04
-#define ACPI_OSI_WIN_XP_SP2 0x05
-#define ACPI_OSI_WINSRV_2003_SP1 0x06
-#define ACPI_OSI_WIN_VISTA 0x07
-#define ACPI_OSI_WINSRV_2008 0x08
-#define ACPI_OSI_WIN_VISTA_SP1 0x09
-#define ACPI_OSI_WIN_VISTA_SP2 0x0A
-#define ACPI_OSI_WIN_7 0x0B
-#define ACPI_OSI_WIN_8 0x0C
-
#define ACPI_ALWAYS_ILLEGAL 0x00
struct acpi_interface_info {
diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
index 6505774..c1638c3 100644
--- a/drivers/acpi/acpica/utxface.c
+++ b/drivers/acpi/acpica/utxface.c
@@ -389,6 +389,25 @@ ACPI_EXPORT_SYMBOL(acpi_install_interface_handler)
/*****************************************************************************
*
+ * FUNCTION: acpi_osi_version
+ *
+ * PARAMETERS: None
+ *
+ * RETURN: Last OS version requested via _OSI
+ *
+ * DESCRIPTION: Returns the argument to the most recent _OSI query performed
+ * by the firmware
+ *
+ ****************************************************************************/
+u8 acpi_osi_version(void)
+{
+ return acpi_gbl_osi_data;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_osi_version)
+
+/*****************************************************************************
+ *
* FUNCTION: acpi_check_address_range
*
* PARAMETERS: space_id - Address space ID
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 454881e..41d3ac1 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -82,6 +82,22 @@ extern u8 acpi_gbl_truncate_io_addresses;
extern u8 acpi_gbl_disable_auto_repair;
/*
+ * Values returned by acpi_osi_version()
+ */
+#define ACPI_OSI_WIN_2000 0x01
+#define ACPI_OSI_WIN_XP 0x02
+#define ACPI_OSI_WIN_XP_SP1 0x03
+#define ACPI_OSI_WINSRV_2003 0x04
+#define ACPI_OSI_WIN_XP_SP2 0x05
+#define ACPI_OSI_WINSRV_2003_SP1 0x06
+#define ACPI_OSI_WIN_VISTA 0x07
+#define ACPI_OSI_WINSRV_2008 0x08
+#define ACPI_OSI_WIN_VISTA_SP1 0x09
+#define ACPI_OSI_WIN_VISTA_SP2 0x0A
+#define ACPI_OSI_WIN_7 0x0B
+#define ACPI_OSI_WIN_8 0x0C
+
+/*
* Hardware-reduced prototypes. All interfaces that use these macros will
* be configured out of the ACPICA build if the ACPI_REDUCED_HARDWARE flag
* is set to TRUE.
@@ -307,6 +323,12 @@ acpi_status acpi_install_notify_handler(acpi_handle device, u32 handler_type,
acpi_notify_handler handler,
void *context);
+#ifdef CONFIG_ACPI
+u8 acpi_osi_version(void);
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
acpi_status
acpi_remove_notify_handler(acpi_handle device,
u32 handler_type, acpi_notify_handler handler);
--
1.8.2.1
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 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.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/gpu/drm/i915/i915_dma.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 3b315ba..23b6292 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
/* Must be done after probing outputs */
intel_opregion_init(dev);
acpi_video_register();
+ /* Don't use ACPI backlight functions on Windows 8 platforms */
+ if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+ acpi_video_backlight_unregister();
}
if (IS_GEN5(dev))
--
1.8.2.1
From: "Lee, Chun-Yi" <[email protected]>
There have some situation we unregister whole acpi/video driver by downstream
driver just want to remove backlight control interface of acpi/video. It caues
we lost other functions of acpi/video, e.g. transfer acpi event to input event.
So, this patch add a new function, find_video_unregister_backlight, it provide
the interface let downstream driver can tell acpi/video to unregister backlight
interface of all acpi video devices. Then we can keep functions of acpi/video
but only remove backlight support.
Reference: bko#35622
https://bugzilla.kernel.org/show_bug.cgi?id=35622
v2: Also unregister cooling devices.
Tested-by: Andrzej Krentosz <[email protected]>
Cc: Zhang Rui <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Carlos Corbacho <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Corentin Chary <[email protected]>
Cc: Aaron Lu <[email protected]>
Cc: Thomas Renninger <[email protected]>
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
drivers/acpi/video.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++----
include/acpi/video.h | 2 ++
2 files changed, 89 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 5b32e15..da08ff7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -43,6 +43,7 @@
#include <acpi/acpi_drivers.h>
#include <linux/suspend.h>
#include <acpi/video.h>
+#include <linux/mutex.h>
#define PREFIX "ACPI: "
@@ -86,6 +87,9 @@ module_param(allow_duplicates, bool, 0644);
static bool use_bios_initial_backlight = 1;
module_param(use_bios_initial_backlight, bool, 0644);
+static bool backlight_disable;
+module_param(backlight_disable, bool, 0644);
+
static int register_count = 0;
static int acpi_video_bus_add(struct acpi_device *device);
static int acpi_video_bus_remove(struct acpi_device *device);
@@ -214,6 +218,8 @@ static const char device_decode[][30] = {
"UNKNOWN",
};
+static DEFINE_MUTEX(backlight_mutex);
+
static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data);
static void acpi_video_device_rebind(struct acpi_video_bus *video);
static void acpi_video_device_bind(struct acpi_video_bus *video,
@@ -882,7 +888,7 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
device->cap._DDC = 1;
}
- if (acpi_video_backlight_support()) {
+ if (acpi_video_backlight_support() || backlight_disable) {
struct backlight_properties props;
struct pci_dev *pdev;
acpi_handle acpi_parent;
@@ -1083,15 +1089,22 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
struct acpi_video_device *data;
struct acpi_video_device_attrib* attribute;
+ mutex_lock(&backlight_mutex);
+
status =
acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
/* Some device omits _ADR, we skip them instead of fail */
- if (ACPI_FAILURE(status))
- return 0;
+ if (ACPI_FAILURE(status)) {
+ status = 0;
+ goto out;
+ }
data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
+ if (!data) {
+ status = -ENOMEM;
+ goto out;
+ }
+
strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
@@ -1156,6 +1169,8 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
list_add_tail(&data->entry, &video->video_device_list);
mutex_unlock(&video->device_list_lock);
+out:
+ mutex_unlock(&backlight_mutex);
return status;
}
@@ -1336,7 +1351,7 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
int result = -EINVAL;
/* no warning message if acpi_backlight=vendor is used */
- if (!acpi_video_backlight_support())
+ if (!acpi_video_backlight_support() || backlight_disable)
return 0;
if (!device->brightness)
@@ -1869,6 +1884,72 @@ static int __init intel_opregion_present(void)
return opregion;
}
+static acpi_status
+find_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;
+ acpi_status status = AE_OK;
+
+ mutex_lock(&backlight_mutex);
+
+ if (acpi_bus_get_device(handle, &acpi_dev))
+ goto out;
+
+ if (!acpi_match_device_ids(acpi_dev, video_device_ids)) {
+ video = acpi_driver_data(acpi_dev);
+
+ if (!video)
+ goto out;
+
+ 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);
+ }
+out:
+ mutex_unlock(&backlight_mutex);
+ return status;
+}
+
+void acpi_video_backlight_unregister(void)
+{
+ if (!register_count) {
+ /*
+ * If the acpi video bus is already unloaded, don't
+ * unregister backlight of devices and return directly.
+ */
+ return;
+ }
+
+ backlight_disable = 1;
+
+ acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+ ACPI_UINT32_MAX, find_video_unregister_backlight,
+ NULL, NULL, NULL);
+ return;
+}
+EXPORT_SYMBOL(acpi_video_backlight_unregister);
+
int acpi_video_register(void)
{
int result = 0;
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 61109f2..1e810a1 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -19,11 +19,13 @@ struct acpi_device;
#if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
extern int acpi_video_register(void);
extern void acpi_video_unregister(void);
+extern void acpi_video_backlight_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 void acpi_video_unregister(void) { return; }
+static inline void acpi_video_backlight_unregister(void) { return; }
static inline int acpi_video_get_edid(struct acpi_device *device, int type,
int device_id, void **edid)
{
--
1.8.2.1
On Sun, Jun 09, 2013 at 07:01:39PM -0400, Matthew Garrett wrote:
> 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 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.
>
> Signed-off-by: Matthew Garrett <[email protected]>
Make sense and I guess it's easier to merge this all through the acpi
tree. So
Acked-by: Daniel Vetter <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 3b315ba..23b6292 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> /* Must be done after probing outputs */
> intel_opregion_init(dev);
> acpi_video_register();
> + /* Don't use ACPI backlight functions on Windows 8 platforms */
> + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> + acpi_video_backlight_unregister();
> }
>
> if (IS_GEN5(dev))
> --
> 1.8.2.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
於 日，2013-06-09 於 19:01 -0400，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 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.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 3b315ba..23b6292 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> /* Must be done after probing outputs */
> intel_opregion_init(dev);
> acpi_video_register();
> + /* Don't use ACPI backlight functions on Windows 8 platforms */
> + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> + acpi_video_backlight_unregister();
> }
>
> if (IS_GEN5(dev))
This patch set works to me on Acer Aspire V3 notebook for unregister the
backlight interface of acpi video driver when i915 loaded. Acer Aspire
V3 has the Windows8 support in DSDT.
Tested-by: Lee, Chun-Yi <[email protected]>
Thanks a lot!
Joey Lee
Hi Matthew,
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
> Windows 8 introduced new policy for backlight control by pushing it out to
> graphics drivers. This appears to have coincided with a range of vendors
> adding Windows 8 checks to their backlight control code which trigger either
> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
> thing to do would be to just disable ACPI backlight control entirely if the
> firmware indicates Windows 8 support, but it's entirely possible that
> individual graphics drivers might still make use of the ACPI functionality in
> preference to native control.
>
> The first two patches in this series are picked from other patchesets aimed at
> solving similar problems. The last simply unregisters ACPI backlight control
> on Windows 8 systems when using an Intel GPU. Similar code could be added to
> other drivers, but I'm reluctant to do so without further investigation as
> to the behaviour of the vendor drivers under Windows.
I happen to know that alternative solution to this problem is being worked on,
so I'm going to wait until it is submitted and then we'll decide what to merge.
I'm slightly concerned about unregistering ACPI backlight control for all
systems declaring win8 support, even though it may actually work for them.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Mon, 2013-06-10 at 13:59 +0200, Rafael J. Wysocki wrote:
> I happen to know that alternative solution to this problem is being worked on,
> so I'm going to wait until it is submitted and then we'll decide what to merge.
Sure.
> I'm slightly concerned about unregistering ACPI backlight control for all
> systems declaring win8 support, even though it may actually work for them.
Right, that's why I think it's correct to leave it up to the graphics
drivers. It seems like they're the ones that make the policy
determination under Windows now. The policy as implemented here may not
be correct, but doing better would probably involve Intel letting us
know how their Windows driver behaves.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Radeon probably needs something similar. See attached untested patch.
Alex
On Sun, Jun 9, 2013 at 7:01 PM, Matthew Garrett
<[email protected]> wrote:
> 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 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.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 3b315ba..23b6292 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> /* Must be done after probing outputs */
> intel_opregion_init(dev);
> acpi_video_register();
> + /* Don't use ACPI backlight functions on Windows 8 platforms */
> + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> + acpi_video_backlight_unregister();
> }
>
> if (IS_GEN5(dev))
> --
> 1.8.2.1
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, Jun 09, 2013 at 07:01:36PM -0400, Matthew Garrett wrote:
> Windows 8 introduced new policy for backlight control by pushing it out to
> graphics drivers. This appears to have coincided with a range of vendors
> adding Windows 8 checks to their backlight control code which trigger either
> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
> thing to do would be to just disable ACPI backlight control entirely if the
> firmware indicates Windows 8 support, but it's entirely possible that
> individual graphics drivers might still make use of the ACPI functionality in
> preference to native control.
>
> The first two patches in this series are picked from other patchesets aimed at
> solving similar problems. The last simply unregisters ACPI backlight control
> on Windows 8 systems when using an Intel GPU. Similar code could be added to
> other drivers, but I'm reluctant to do so without further investigation as
> to the behaviour of the vendor drivers under Windows.
I've received some feedback from user testing of these patches (don't
have an affected machine myself) and the feedback is mostly good. One
user reported it didn't work, but I that machine may have discrete
graphics (waiting to hear back from the user).
The main drawback I see with any approach like this one is that the
backlight remains broken for users of proprietary graphics drivers.
Seth
On 06/10/2013 07:01 AM, Matthew Garrett wrote:
> 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 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.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 3b315ba..23b6292 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> /* Must be done after probing outputs */
> intel_opregion_init(dev);
> acpi_video_register();
> + /* Don't use ACPI backlight functions on Windows 8 platforms */
> + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> + acpi_video_backlight_unregister();
What about the platform driver created backlight interface? Oh right,
thinkpad_acpi will not create backlight interface for them since they
are not in DMI table, so acpi_video_backlight_support() will return
true and thinkspad_acpi thinks ACPI video driver is controlling
backlight so it will not create backlight interface for them.
Then we will need to remember not to add any of those systems into
the DMI table of video_detect.c, or thinkpad_acpi module will create
backlight interface for them and according to reporter, that interface
does not work either.
What about a priority based solution? We can introduce a new field named
priority to backlight_device and instead of calling another module's
function like the unregister one here(which cause unnecessary module
dependency), we only need to boost priority for its own interface. This
field will be exported to sysfs, so user can change it during runtime
too. And we can also introduce a new kernel command line as
backlight.force_interface=raw/firmware/platform, to overcome the limited
functionality provided by acpi_backlight=video/vendor, which does not
involve GPU's interface.
And we can place the quirk code in backlight layer instead of individual
backlight functionality provider module. Suppose we have a backlight
manager there, for all win8 systems, we can boost the raw type's
priority on its registration, so no need to add code in
intel/amd/etc./'s GPU driver code.
With priority based solution, all backlight control interfaces stay,
the priority field is an indication given by kernel to user space.
For a complete description on backlight control background and the
proposed solution, please take a look at:
https://gist.github.com/aaronlu/5779828
It would be good to know your opinions, thanks.
-Aaron
On Fri, 2013-06-14 at 14:47 +0800, Aaron Lu wrote:
> What about a priority based solution? We can introduce a new field named
> priority to backlight_device and instead of calling another module's
> function like the unregister one here(which cause unnecessary module
> dependency), we only need to boost priority for its own interface. This
> field will be exported to sysfs, so user can change it during runtime
> too. And we can also introduce a new kernel command line as
> backlight.force_interface=raw/firmware/platform, to overcome the limited
> functionality provided by acpi_backlight=video/vendor, which does not
> involve GPU's interface.
How would that work with existing userspace?
> And we can place the quirk code in backlight layer instead of individual
> backlight functionality provider module. Suppose we have a backlight
> manager there, for all win8 systems, we can boost the raw type's
> priority on its registration, so no need to add code in
> intel/amd/etc./'s GPU driver code.
But we'd need to add code to every piece of userspace that currently
uses the backlight, right?
> With priority based solution, all backlight control interfaces stay,
> the priority field is an indication given by kernel to user space.
We shouldn't export interfaces if we don't expect them to work.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 06/15/2013 01:29 AM, Matthew Garrett wrote:
> On Fri, 2013-06-14 at 14:47 +0800, Aaron Lu wrote:
>
>> What about a priority based solution? We can introduce a new field named
>> priority to backlight_device and instead of calling another module's
>> function like the unregister one here(which cause unnecessary module
>> dependency), we only need to boost priority for its own interface. This
>> field will be exported to sysfs, so user can change it during runtime
>> too. And we can also introduce a new kernel command line as
>> backlight.force_interface=raw/firmware/platform, to overcome the limited
>> functionality provided by acpi_backlight=video/vendor, which does not
>> involve GPU's interface.
>
> How would that work with existing userspace?
User space tool will need to be updated to use this as stated in the
gist page, I've patches for gsd-backlight-helper and xorg-x11-drv-intel,
for others we can add I think if the priority based solution is deemed
useful.
>
>> And we can place the quirk code in backlight layer instead of individual
>> backlight functionality provider module. Suppose we have a backlight
>> manager there, for all win8 systems, we can boost the raw type's
>> priority on its registration, so no need to add code in
>> intel/amd/etc./'s GPU driver code.
>
> But we'd need to add code to every piece of userspace that currently
> uses the backlight, right?
Yes that's the case.
>
>> With priority based solution, all backlight control interfaces stay,
>> the priority field is an indication given by kernel to user space.
>
> We shouldn't export interfaces if we don't expect them to work.
It's not easy to decide if they work or not sometimes, e.g. I came
across a system that claims win8 in ACPI table and has an Intel GPU,
while its ACPI video interface also works. With this patch, the working
ACPI video interface is removed, while with the priority based solution,
the GPU's interface priority gets higher, but the ACPI video interface
still stays.
Thanks,
Aaron
On Sat, 2013-06-15 at 09:26 +0800, Aaron Lu wrote:
> On 06/15/2013 01:29 AM, Matthew Garrett wrote:
> > How would that work with existing userspace?
>
> User space tool will need to be updated to use this as stated in the
> gist page, I've patches for gsd-backlight-helper and xorg-x11-drv-intel,
> for others we can add I think if the priority based solution is deemed
> useful.
Right, that's not a great solution.
> > We shouldn't export interfaces if we don't expect them to work.
>
> It's not easy to decide if they work or not sometimes, e.g. I came
> across a system that claims win8 in ACPI table and has an Intel GPU,
> while its ACPI video interface also works. With this patch, the working
> ACPI video interface is removed, while with the priority based solution,
> the GPU's interface priority gets higher, but the ACPI video interface
> still stays.
Well, Windows 8 will only use the ACPI backlight interface if the GPU
driver decides to, right? So the logic for deciding whether to remove
the ACPI backlight control or not should be left up to the GPU. There's
no harm in refusing to expose a working method if there's another
working method, but there is harm in exposing a broken one and expecting
userspace to know the difference.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 06/15/2013 09:38 AM, Matthew Garrett wrote:
> On Sat, 2013-06-15 at 09:26 +0800, Aaron Lu wrote:
>> It's not easy to decide if they work or not sometimes, e.g. I came
>> across a system that claims win8 in ACPI table and has an Intel GPU,
>> while its ACPI video interface also works. With this patch, the working
>> ACPI video interface is removed, while with the priority based solution,
>> the GPU's interface priority gets higher, but the ACPI video interface
>> still stays.
>
> Well, Windows 8 will only use the ACPI backlight interface if the GPU
> driver decides to, right? So the logic for deciding whether to remove
> the ACPI backlight control or not should be left up to the GPU. There's
I don't know this. From the document I googled, Microsoft suggests under
win8, backlight should be controlled by the graphics driver for smooth
brightness level change, instead of ACPI or other methods. So it is
possible that OEM will not test the ACPI interface well and thus the
interface is likely broken. I don't see why GPU driver has any better
knowledge on which systems the firmware interface is broken or not.
> no harm in refusing to expose a working method if there's another
> working method, but there is harm in exposing a broken one and expecting
> userspace to know the difference.
BTW, the proposed solution is not meant to solve win8 problems alone, it
should make solving other problems easy and make individual backlight
control interface provider module independent with each other such as
the platform drivers will not need to check if ACPI video driver will
control backlight or not and can always create backlight interface(its
default priority is lower that ACPI video driver's so will not be taken
by user space by default, showing the same behavior of the current code).
The current acpi_backlight=video/vendor kernel command line is pretty
misleading, for laptops that do not have vendor backlight interface,
adding acpi_backlight=vendor actually makes the system using the GPU's
interface. Some laptops are using this switch to work around problems in
ACPI video driver and users think they are using vendor interface.
That's why I think we need a new command line as the
backlight.force_interface=raw/firmware/platform.
Instead of letting individual driver to make decisions on which
backlight interface this system should use(either in platform driver as
we currently did, see acer-wmi and asus-wmi, or GPU driver as this case),
I think we need a better and clear way to handle such things. For
example, suppose an acer laptop: vendor does not support backlight, ACPI
video's backlight interface is broken and GPU's works, to make it work,
user will need to select acer-wmi module while this module does not have
anything to do with the functionality, but simply because it serves as
the backlight manager for acer laptops.
The above information and idea is formed while solving bugs reported
in kernel bugzilla video component, the proposed solutin may not be good
enough, but I hope we can find a better way to handle backlight problems.
Thanks,
Aaron
On Sat, Jun 15, 2013 at 12:14:42PM +0800, Aaron Lu wrote:
> On 06/15/2013 09:38 AM, Matthew Garrett wrote:
> > Well, Windows 8 will only use the ACPI backlight interface if the GPU
> > driver decides to, right? So the logic for deciding whether to remove
> > the ACPI backlight control or not should be left up to the GPU. There's
>
> I don't know this. From the document I googled, Microsoft suggests under
> win8, backlight should be controlled by the graphics driver for smooth
> brightness level change, instead of ACPI or other methods. So it is
> possible that OEM will not test the ACPI interface well and thus the
> interface is likely broken. I don't see why GPU driver has any better
> knowledge on which systems the firmware interface is broken or not.
The vendor will presumably have tested that backlight control works - if
the GPU driver uses the ACPI interface and backlight control is broken,
then the vendor would fix it.
> > no harm in refusing to expose a working method if there's another
> > working method, but there is harm in exposing a broken one and expecting
> > userspace to know the difference.
>
> BTW, the proposed solution is not meant to solve win8 problems alone, it
> should make solving other problems easy and make individual backlight
> control interface provider module independent with each other such as
> the platform drivers will not need to check if ACPI video driver will
> control backlight or not and can always create backlight interface(its
> default priority is lower that ACPI video driver's so will not be taken
> by user space by default, showing the same behavior of the current code).
Sure, but it still requires the replacement of existing userspace. We
need to fix the problem with existing userspace, too.
> The current acpi_backlight=video/vendor kernel command line is pretty
> misleading, for laptops that do not have vendor backlight interface,
> adding acpi_backlight=vendor actually makes the system using the GPU's
> interface. Some laptops are using this switch to work around problems in
> ACPI video driver and users think they are using vendor interface.
> That's why I think we need a new command line as the
> backlight.force_interface=raw/firmware/platform.
No, I think we need to fix the bugs that currently require users to pass
options.
> Instead of letting individual driver to make decisions on which
> backlight interface this system should use(either in platform driver as
> we currently did, see acer-wmi and asus-wmi, or GPU driver as this case),
> I think we need a better and clear way to handle such things. For
> example, suppose an acer laptop: vendor does not support backlight, ACPI
> video's backlight interface is broken and GPU's works, to make it work,
> user will need to select acer-wmi module while this module does not have
> anything to do with the functionality, but simply because it serves as
> the backlight manager for acer laptops.
How do these machines work under Windows? Until we know the answer to
that, we don't know what the correct way to handle the problem is under
Linux.
--
Matthew Garrett | [email protected]
On 06/15/2013 12:19 PM, Matthew Garrett wrote:
> On Sat, Jun 15, 2013 at 12:14:42PM +0800, Aaron Lu wrote:
>> On 06/15/2013 09:38 AM, Matthew Garrett wrote:
>>> Well, Windows 8 will only use the ACPI backlight interface if the GPU
>>> driver decides to, right? So the logic for deciding whether to remove
>>> the ACPI backlight control or not should be left up to the GPU. There's
>>
>> I don't know this. From the document I googled, Microsoft suggests under
>> win8, backlight should be controlled by the graphics driver for smooth
>> brightness level change, instead of ACPI or other methods. So it is
>> possible that OEM will not test the ACPI interface well and thus the
>> interface is likely broken. I don't see why GPU driver has any better
>> knowledge on which systems the firmware interface is broken or not.
>
> The vendor will presumably have tested that backlight control works - if
> the GPU driver uses the ACPI interface and backlight control is broken,
> then the vendor would fix it.
I don't think GPU driver uses ACPI interface, that's why for some
systems, ACPI interface is broken while the GPU's is not.
>
>>> no harm in refusing to expose a working method if there's another
>>> working method, but there is harm in exposing a broken one and expecting
>>> userspace to know the difference.
>>
>> BTW, the proposed solution is not meant to solve win8 problems alone, it
>> should make solving other problems easy and make individual backlight
>> control interface provider module independent with each other such as
>> the platform drivers will not need to check if ACPI video driver will
>> control backlight or not and can always create backlight interface(its
>> default priority is lower that ACPI video driver's so will not be taken
>> by user space by default, showing the same behavior of the current code).
>
> Sure, but it still requires the replacement of existing userspace. We
> need to fix the problem with existing userspace, too.
Yes. The problem is two way. First, some of the backlight interface
privoder module provides a broken interface; Two, the userspace picked a
broken interface while there are usable ones. For example, in the win8
thinkpad case, the ACPI video driver provides broken interface and the
GPU driver provides usable interface, but userspace choose ACPI video's
interface. To make things complicated, if we make the broken interface
disappear in ACPI video module, then the platform driver thinkpad_acpi
will start to provide another broken interface. I have to say, solve it
in the GPU code is a clever way, it's just a little weird to me for a
broken interface to be blacklisted, we have to do it in another module,
not the broken module itself.
>
>> The current acpi_backlight=video/vendor kernel command line is pretty
>> misleading, for laptops that do not have vendor backlight interface,
>> adding acpi_backlight=vendor actually makes the system using the GPU's
>> interface. Some laptops are using this switch to work around problems in
>> ACPI video driver and users think they are using vendor interface.
>> That's why I think we need a new command line as the
>> backlight.force_interface=raw/firmware/platform.
>
> No, I think we need to fix the bugs that currently require users to pass
> options.
For ACPI video driver, since it relies on firmware to do the right thing,
if the functionality is broken, then it is, there is hardly anything we
can do...(not always, we can quirk in some cases, but there are cases we
can do nothing). In this case, we can:
1 Ask user to add acpi_backlight=vendor, so that ACPI video driver will
not create backlight interface and userspace will not pick it;
2 Add that model in DMI table in video_detect.c, eliminating the need for
that command line;
3 Add that model in DMI table in another module, let that module
unregister backlight interface provided by ACPI video as is done in
acer-wmi, asus-wmi, and i915 in this case;
3 Use the backlight section in xorg.conf to make X uses another
backlight interface. This may not work for distros that use
gsd-backlight-helper, which always prefers firmware.
Which one do you prefer?
>
>> Instead of letting individual driver to make decisions on which
>> backlight interface this system should use(either in platform driver as
>> we currently did, see acer-wmi and asus-wmi, or GPU driver as this case),
>> I think we need a better and clear way to handle such things. For
>> example, suppose an acer laptop: vendor does not support backlight, ACPI
>> video's backlight interface is broken and GPU's works, to make it work,
>> user will need to select acer-wmi module while this module does not have
>> anything to do with the functionality, but simply because it serves as
>> the backlight manager for acer laptops.
>
> How do these machines work under Windows? Until we know the answer to
> that, we don't know what the correct way to handle the problem is under
> Linux.
Do you mean we need to understand Windows behavior so that we can
decide where to place the code to do the backlight management thing?
So for win8 case, we know MS will use GPU driver to do backlight
control, all this means to me is, ACPI video's and platform driver's
interface are more likely broken on those systems, but it doesn't
qualify why Linux' GPU driver should do the decision, it's not that the
GPU driver can poke some GPU register and then decide oh, this model
does not have working ACPI backlight interface. As this patch shows, we
make the decision by OSI, which is not specific to GPU driver.
BTW, I don't think any module knows something better than another
module, all quirk logic and DMI entry we currently have in Linux are
got by user's feedback(bug reports), it doesn't seem to me some module
is natrually the one that should make this decision. Or do I miss
something?
Thanks,
Aaron
On Sat, Jun 15, 2013 at 08:29:15PM +0800, Aaron Lu wrote:
> On 06/15/2013 12:19 PM, Matthew Garrett wrote:
> > The vendor will presumably have tested that backlight control works - if
> > the GPU driver uses the ACPI interface and backlight control is broken,
> > then the vendor would fix it.
>
> I don't think GPU driver uses ACPI interface, that's why for some
> systems, ACPI interface is broken while the GPU's is not.
On systems where the ACPI interface is broken, the GPU driver clearly
doesn't use the ACPI interface. As yet, we don't know if that's always
true, though.
> > Sure, but it still requires the replacement of existing userspace. We
> > need to fix the problem with existing userspace, too.
>
> Yes. The problem is two way. First, some of the backlight interface
> privoder module provides a broken interface; Two, the userspace picked a
> broken interface while there are usable ones. For example, in the win8
> thinkpad case, the ACPI video driver provides broken interface and the
> GPU driver provides usable interface, but userspace choose ACPI video's
> interface. To make things complicated, if we make the broken interface
> disappear in ACPI video module, then the platform driver thinkpad_acpi
> will start to provide another broken interface. I have to say, solve it
> in the GPU code is a clever way, it's just a little weird to me for a
> broken interface to be blacklisted, we have to do it in another module,
> not the broken module itself.
The ACPI driver has no way of making this kind of policy decision. The
GPU driver does.
> > No, I think we need to fix the bugs that currently require users to pass
> > options.
>
> For ACPI video driver, since it relies on firmware to do the right thing,
> if the functionality is broken, then it is, there is hardly anything we
> can do...(not always, we can quirk in some cases, but there are cases we
> can do nothing). In this case, we can:
> 1 Ask user to add acpi_backlight=vendor, so that ACPI video driver will
> not create backlight interface and userspace will not pick it;
> 2 Add that model in DMI table in video_detect.c, eliminating the need for
> that command line;
> 3 Add that model in DMI table in another module, let that module
> unregister backlight interface provided by ACPI video as is done in
> acer-wmi, asus-wmi, and i915 in this case;
> 3 Use the backlight section in xorg.conf to make X uses another
> backlight interface. This may not work for distros that use
> gsd-backlight-helper, which always prefers firmware.
>
> Which one do you prefer?
I'd prefer to figure out how it works in Windows and implement it the
same way.
> > How do these machines work under Windows? Until we know the answer to
> > that, we don't know what the correct way to handle the problem is under
> > Linux.
>
> Do you mean we need to understand Windows behavior so that we can
> decide where to place the code to do the backlight management thing?
> So for win8 case, we know MS will use GPU driver to do backlight
> control, all this means to me is, ACPI video's and platform driver's
> interface are more likely broken on those systems, but it doesn't
> qualify why Linux' GPU driver should do the decision, it's not that the
> GPU driver can poke some GPU register and then decide oh, this model
> does not have working ACPI backlight interface. As this patch shows, we
> make the decision by OSI, which is not specific to GPU driver.
Sure it is - for all we know there's a value in the opregion space that
tells the Intel GPU driver which interface we should be using. We don't
know because Intel haven't released a version of the opregion driver
newer than 2007. It may be that all Windows 8 GPU drivers use the GPU
backlight control registers directly and never use any firmware
interface, but right now we simply don't know that. All we can do is
look at the existing cases we have and say that it appears that
Intel graphics systems don't use the ACPI interface. Anything more than
that requires more evidence.
> BTW, I don't think any module knows something better than another
> module, all quirk logic and DMI entry we currently have in Linux are
> got by user's feedback(bug reports), it doesn't seem to me some module
> is natrually the one that should make this decision. Or do I miss
> something?
The GPU driver makes the policy decision under Windows 8, so it's the
natural place to make this decision on Windows 8 systems.
--
Matthew Garrett | [email protected]
Hi all,
So to me it looks like the discussion is going in circles a bit, hence let
me drop my maintainer-opinion here:
1. Matthew's patch series here looks reasonable, and if it fixes a bunch
of systems (which it seems to) it has my Ack and imo should go in. If acpi
maintainers can smash their Ack onto the acpi parts I'd very much like to
merge this into drm-intel-next, that should give us the most coverage for
intel systems.
Len, Rafael, are you ok with the acpi part of this and merging it through
drm-intel-next?
2. Imo the current amount of quirking we expose to users (we have kernel
options to disable the acpi interface, blacklist platform modules, all
backlights can be tested through sysfs and on top of that xf86-video-intel
has an xorg.conf to select the backlight used by the driver). I haven't
spotted a compelling reason in this thread to add another one, what we
have seems to be good enough to debug backligh issues.
3. Also, adding yet another backlight quirk mechanism isn't gonna
magically fix broken systems.
We _really_ should strive to make this just work and not offer the angry
user another roll of duct-tape for free.
4. The currently established priority selection for backlights of platform
> firmware > raw seems to be good enough. Note that the explicit list in
xf86-vidoe-intel is only used as a fallback for really old kernels which
do not expose this information. We could probably rip it out.
5. We've recently looked at opregion again and couldn't spot a hint.
Unfortnately it looks like both noodling better information out of Intel
and trying to publish an updated opregion spec seem to be losing games :(
We'll keep on trying though.
Aside at the end: If the gnome tool indeed has its own backlight code and
doesn't just use that as a fallback if the xrandr backligh property isn't
available, then that's just a serious bug in gnome and should be fixed
asap. But imo that's not something we should try to (nor do I see any way
how to) work around in the kernel.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sat, Jun 15, 2013 at 08:29:42PM +0200, Daniel Vetter wrote:
> Aside at the end: If the gnome tool indeed has its own backlight code and
> doesn't just use that as a fallback if the xrandr backligh property isn't
> available, then that's just a serious bug in gnome and should be fixed
> asap. But imo that's not something we should try to (nor do I see any way
> how to) work around in the kernel.
It's only used if there's no backlight property on the display.
--
Matthew Garrett | [email protected]
On Saturday, June 15, 2013 08:29:42 PM Daniel Vetter wrote:
> Hi all,
>
> So to me it looks like the discussion is going in circles a bit, hence let
> me drop my maintainer-opinion here:
>
> 1. Matthew's patch series here looks reasonable, and if it fixes a bunch
> of systems (which it seems to) it has my Ack and imo should go in. If acpi
> maintainers can smash their Ack onto the acpi parts I'd very much like to
> merge this into drm-intel-next, that should give us the most coverage for
> intel systems.
>
> Len, Rafael, are you ok with the acpi part of this and merging it through
> drm-intel-next?
It has to go through the ACPI tree because of the ACPICA patch that needs to
be synchronized with the ACPICA upstream. Sorry.
That said, I'm going to take this patchset.
> 2. Imo the current amount of quirking we expose to users (we have kernel
> options to disable the acpi interface, blacklist platform modules, all
> backlights can be tested through sysfs and on top of that xf86-video-intel
> has an xorg.conf to select the backlight used by the driver). I haven't
> spotted a compelling reason in this thread to add another one, what we
> have seems to be good enough to debug backligh issues.
>
> 3. Also, adding yet another backlight quirk mechanism isn't gonna
> magically fix broken systems.
>
> We _really_ should strive to make this just work and not offer the angry
> user another roll of duct-tape for free.
>
> 4. The currently established priority selection for backlights of platform
> > firmware > raw seems to be good enough. Note that the explicit list in
> xf86-vidoe-intel is only used as a fallback for really old kernels which
> do not expose this information. We could probably rip it out.
>
> 5. We've recently looked at opregion again and couldn't spot a hint.
> Unfortnately it looks like both noodling better information out of Intel
> and trying to publish an updated opregion spec seem to be losing games :(
> We'll keep on trying though.
>
> Aside at the end: If the gnome tool indeed has its own backlight code and
> doesn't just use that as a fallback if the xrandr backligh property isn't
> available, then that's just a serious bug in gnome and should be fixed
> asap. But imo that's not something we should try to (nor do I see any way
> how to) work around in the kernel.
Agreed.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Sat, Jun 15, 2013 at 10:27:06PM +0200, Rafael J. Wysocki wrote:
> On Saturday, June 15, 2013 08:29:42 PM Daniel Vetter wrote:
> > Hi all,
> >
> > So to me it looks like the discussion is going in circles a bit, hence let
> > me drop my maintainer-opinion here:
> >
> > 1. Matthew's patch series here looks reasonable, and if it fixes a bunch
> > of systems (which it seems to) it has my Ack and imo should go in. If acpi
> > maintainers can smash their Ack onto the acpi parts I'd very much like to
> > merge this into drm-intel-next, that should give us the most coverage for
> > intel systems.
> >
> > Len, Rafael, are you ok with the acpi part of this and merging it through
> > drm-intel-next?
>
> It has to go through the ACPI tree because of the ACPICA patch that needs to
> be synchronized with the ACPICA upstream. Sorry.
No problem, since we're pretty close to the merge window that would have
at most resulted in a few more weeks of testing in i915 trees anyway. -rc
kernels should still give us plenty of time to catch fallout, if there is
any.
> That said, I'm going to take this patchset.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Matthew,
On Sunday, June 09, 2013 07:01:38 PM Matthew Garrett wrote:
> Drivers may need to make policy decisions based on the OS that the firmware
> believes it's interacting with. ACPI firmware will make a series of _OSI
> calls, starting from the oldest OS version they support and ending with the
> most recent. Add a function to return the last successful call so that
> drivers know what the firmware's expecting.
>
> Based on a patch by Seth Forshee <[email protected]>
Bob (CCed) would prefer us to access acpi_gbl_osi_data directly instead of
adding the wrapper to ACPICA. He also thinks that the symbol definitions
should go into include/acpi/actypes.h rather than into acpixf.h.
Then, the only ACPICA change would be to move the symbols and we can
add a Linux-specific patch on top of that adding the acpi_gbl_osi_data
wrapper.
How does that sound?
Rafael
> Signed-off-by: Matthew Garrett <[email protected]>
> Cc: Seth Forshee <[email protected]>
> ---
> drivers/acpi/acpica/aclocal.h | 13 -------------
> drivers/acpi/acpica/utxface.c | 19 +++++++++++++++++++
> include/acpi/acpixf.h | 22 ++++++++++++++++++++++
> 3 files changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
> index d5bfbd3..8a2f532 100644
> --- a/drivers/acpi/acpica/aclocal.h
> +++ b/drivers/acpi/acpica/aclocal.h
> @@ -948,19 +948,6 @@ struct acpi_bit_register_info {
>
> /* Structs and definitions for _OSI support and I/O port validation */
>
> -#define ACPI_OSI_WIN_2000 0x01
> -#define ACPI_OSI_WIN_XP 0x02
> -#define ACPI_OSI_WIN_XP_SP1 0x03
> -#define ACPI_OSI_WINSRV_2003 0x04
> -#define ACPI_OSI_WIN_XP_SP2 0x05
> -#define ACPI_OSI_WINSRV_2003_SP1 0x06
> -#define ACPI_OSI_WIN_VISTA 0x07
> -#define ACPI_OSI_WINSRV_2008 0x08
> -#define ACPI_OSI_WIN_VISTA_SP1 0x09
> -#define ACPI_OSI_WIN_VISTA_SP2 0x0A
> -#define ACPI_OSI_WIN_7 0x0B
> -#define ACPI_OSI_WIN_8 0x0C
> -
> #define ACPI_ALWAYS_ILLEGAL 0x00
>
> struct acpi_interface_info {
> diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
> index 6505774..c1638c3 100644
> --- a/drivers/acpi/acpica/utxface.c
> +++ b/drivers/acpi/acpica/utxface.c
> @@ -389,6 +389,25 @@ ACPI_EXPORT_SYMBOL(acpi_install_interface_handler)
>
> /*****************************************************************************
> *
> + * FUNCTION: acpi_osi_version
> + *
> + * PARAMETERS: None
> + *
> + * RETURN: Last OS version requested via _OSI
> + *
> + * DESCRIPTION: Returns the argument to the most recent _OSI query performed
> + * by the firmware
> + *
> + ****************************************************************************/
> +u8 acpi_osi_version(void)
> +{
> + return acpi_gbl_osi_data;
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_osi_version)
> +
> +/*****************************************************************************
> + *
> * FUNCTION: acpi_check_address_range
> *
> * PARAMETERS: space_id - Address space ID
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 454881e..41d3ac1 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -82,6 +82,22 @@ extern u8 acpi_gbl_truncate_io_addresses;
> extern u8 acpi_gbl_disable_auto_repair;
>
> /*
> + * Values returned by acpi_osi_version()
> + */
> +#define ACPI_OSI_WIN_2000 0x01
> +#define ACPI_OSI_WIN_XP 0x02
> +#define ACPI_OSI_WIN_XP_SP1 0x03
> +#define ACPI_OSI_WINSRV_2003 0x04
> +#define ACPI_OSI_WIN_XP_SP2 0x05
> +#define ACPI_OSI_WINSRV_2003_SP1 0x06
> +#define ACPI_OSI_WIN_VISTA 0x07
> +#define ACPI_OSI_WINSRV_2008 0x08
> +#define ACPI_OSI_WIN_VISTA_SP1 0x09
> +#define ACPI_OSI_WIN_VISTA_SP2 0x0A
> +#define ACPI_OSI_WIN_7 0x0B
> +#define ACPI_OSI_WIN_8 0x0C
> +
> +/*
> * Hardware-reduced prototypes. All interfaces that use these macros will
> * be configured out of the ACPICA build if the ACPI_REDUCED_HARDWARE flag
> * is set to TRUE.
> @@ -307,6 +323,12 @@ acpi_status acpi_install_notify_handler(acpi_handle device, u32 handler_type,
> acpi_notify_handler handler,
> void *context);
>
> +#ifdef CONFIG_ACPI
> +u8 acpi_osi_version(void);
> +#else
> +static inline u8 acpi_osi_version(void) { return 0; }
> +#endif
> +
> acpi_status
> acpi_remove_notify_handler(acpi_handle device,
> u32 handler_type, acpi_notify_handler handler);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Tue, 2013-06-18 at 00:31 +0200, Rafael J. Wysocki wrote:
> Hi Matthew,
>
> On Sunday, June 09, 2013 07:01:38 PM Matthew Garrett wrote:
> > Drivers may need to make policy decisions based on the OS that the firmware
> > believes it's interacting with. ACPI firmware will make a series of _OSI
> > calls, starting from the oldest OS version they support and ending with the
> > most recent. Add a function to return the last successful call so that
> > drivers know what the firmware's expecting.
> >
> > Based on a patch by Seth Forshee <[email protected]>
>
> Bob (CCed) would prefer us to access acpi_gbl_osi_data directly instead of
> adding the wrapper to ACPICA. He also thinks that the symbol definitions
> should go into include/acpi/actypes.h rather than into acpixf.h.
>
> Then, the only ACPICA change would be to move the symbols and we can
> add a Linux-specific patch on top of that adding the acpi_gbl_osi_data
> wrapper.
That sounds good to me. Do you want to respin that, or should I send an
updated set?
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Monday, June 17, 2013 10:37:10 PM Matthew Garrett wrote:
> On Tue, 2013-06-18 at 00:31 +0200, Rafael J. Wysocki wrote:
> > Hi Matthew,
> >
> > On Sunday, June 09, 2013 07:01:38 PM Matthew Garrett wrote:
> > > Drivers may need to make policy decisions based on the OS that the firmware
> > > believes it's interacting with. ACPI firmware will make a series of _OSI
> > > calls, starting from the oldest OS version they support and ending with the
> > > most recent. Add a function to return the last successful call so that
> > > drivers know what the firmware's expecting.
> > >
> > > Based on a patch by Seth Forshee <[email protected]>
> >
> > Bob (CCed) would prefer us to access acpi_gbl_osi_data directly instead of
> > adding the wrapper to ACPICA. He also thinks that the symbol definitions
> > should go into include/acpi/actypes.h rather than into acpixf.h.
> >
> > Then, the only ACPICA change would be to move the symbols and we can
> > add a Linux-specific patch on top of that adding the acpi_gbl_osi_data
> > wrapper.
>
> That sounds good to me. Do you want to respin that, or should I send an
> updated set?
That one should be fine, no need to respin [1/3] and [3/3].
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote:
> The first two patches in this series are picked from other patchesets aimed at
> solving similar problems. The last simply unregisters ACPI backlight control
> on Windows 8 systems when using an Intel GPU. Similar code could be added to
> other drivers, but I'm reluctant to do so without further investigation as
> to the behaviour of the vendor drivers under Windows.
Hi,
I've read this thread coming from
https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches
on a Lenovo ThinkPad X230 with intel graphics.
The problem with thoses fixes is that they still introduce a regression
in how the brightness is handled, at least for me.
Before Linux support for acpi_osi("Windows 2012") (and when booting with
acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
just fine, whether in console, in the display manager or in my desktop
environment (Xfce). xfce4-power-manager just needs to be told that the
brightness keys are already handled and it doesn't need to do anything.
After Linux started responding to Win8 ACPI checks, it somehow broke,
but not completely. Brightness keys are not handled by the kernel
anymore (so no brightness adjustment in console or without a hotkey
daemon running). If I run xfce4-power-manager and tell it to handle the
brightness keys, it does work (although the number of brightness levels
is not completely right). That means both acpi_video0 and
intel_backlight backlight interfaces work, they just don't have the same
precision and max_brightness (more details on the bug).
When adding those three patches, well, acpi_video0 is removed (as
intended), but I still don't have brightness handling in the kernel and
need to have something handling it in userspace.
I really think this is a bad idea. In some cases it might be the only
solution, but having a place where this is set consistently would be
really best, imho. Every userspace daemon might do things differently,
and not everyone even has it. And I'm really not sure brightness keys
should be handled by a desktop environment anyway.
So can the previous behavior be actually restored? Maybe it was easier
to pass the brightness keys information from thinkpad_acpi.ko to
video.ko than it is to i915.ko but since it goes to the input layer
anyway I guess it could be fed to module handling that in a way or
another?
Please keep me on CC: on replies, I'm not subscribed to the various
lists.
Regards,
--
Yves-Alexis
On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
> Before Linux support for acpi_osi("Windows 2012") (and when booting with
> acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> just fine, whether in console, in the display manager or in my desktop
> environment (Xfce). xfce4-power-manager just needs to be told that the
> brightness keys are already handled and it doesn't need to do anything.
Right, the kernel has special-casing to hook the backlight keys up to
the ACPI backlight control. This is an awful thing, because there's no
way to detect this case other than parsing a single driver-specific
module parameter.
Could this functionality be duplicated across other backlight drivers?
Not easily. The ACPI driver receives keypresses and performs backlight
control. The i915 driver doesn't receive keypresses. We could easily tie
certain keycodes into backlight events, but which backlight should they
control? You're really starting to get into the kind of complex policy
decision that's best left to userspace, which is where it should have
been to begin with.
--
Matthew Garrett | [email protected]
On Tue, Jun 25, 2013 at 6:08 PM, Matthew Garrett <[email protected]> wrote:
> On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
>
>> Before Linux support for acpi_osi("Windows 2012") (and when booting with
>> acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
>> just fine, whether in console, in the display manager or in my desktop
>> environment (Xfce). xfce4-power-manager just needs to be told that the
>> brightness keys are already handled and it doesn't need to do anything.
>
> Right, the kernel has special-casing to hook the backlight keys up to
> the ACPI backlight control. This is an awful thing, because there's no
> way to detect this case other than parsing a single driver-specific
> module parameter.
>
> Could this functionality be duplicated across other backlight drivers?
> Not easily. The ACPI driver receives keypresses and performs backlight
> control. The i915 driver doesn't receive keypresses. We could easily tie
> certain keycodes into backlight events, but which backlight should they
> control? You're really starting to get into the kind of complex policy
> decision that's best left to userspace, which is where it should have
> been to begin with.
Do we have any chances to amend this mistake by (gradually) phasing
out support for the direct keypress forwarding in ACPI? Or at least
some plans?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jun 25, 2013 at 06:10:35PM +0200, Daniel Vetter wrote:
> Do we have any chances to amend this mistake by (gradually) phasing
> out support for the direct keypress forwarding in ACPI? Or at least
> some plans?
We could make the default value of brightness_switch_enabled a config
variable and encourage distributions to flip their behaviour.
--
Matthew Garrett | [email protected]
On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
>
> > Before Linux support for acpi_osi("Windows 2012") (and when booting with
> > acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> > just fine, whether in console, in the display manager or in my desktop
> > environment (Xfce). xfce4-power-manager just needs to be told that the
> > brightness keys are already handled and it doesn't need to do anything.
>
> Right, the kernel has special-casing to hook the backlight keys up to
> the ACPI backlight control. This is an awful thing, because there's no
> way to detect this case other than parsing a single driver-specific
> module parameter.
I'm not sure what that means. To detect what case exactly? That the
brightness is handled by video.ko?
>
> Could this functionality be duplicated across other backlight drivers?
> Not easily. The ACPI driver receives keypresses and performs backlight
> control. The i915 driver doesn't receive keypresses. We could easily tie
> certain keycodes into backlight events, but which backlight should they
> control? You're really starting to get into the kind of complex policy
> decision that's best left to userspace, which is where it should have
> been to begin with.
>
Well, I get the reasoning, but I'm not sure I agree. That means
userspace behavior is inconsistent depending on who does it
(gnome-power-manager, gnome-setting-daemon, whatever), and it usually
means there's nothing handling the brightness before those are running,
not to mention people not running them because they don't run a full
blown desktop environment (until someone starts thinking it's a good
idea to handle brightness in systemd).
And in the end, the user just want the brightness keys to correctly
handle the brightness, full stop. Having multiple brightness daemons
using different policies on different hardware is a nightmare for the
end user, imho. From a user point of view, having it handled all in the
kernel works really pretty fine and is completely transparent (I have to
admit that from that point of view, it was even better when it was
handled by the EC but those times seem long gone).
Regards,
--
Yves-Alexis
On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> > Right, the kernel has special-casing to hook the backlight keys up to
> > the ACPI backlight control. This is an awful thing, because there's no
> > way to detect this case other than parsing a single driver-specific
> > module parameter.
>
> I'm not sure what that means. To detect what case exactly? That the
> brightness is handled by video.ko?
That the kernel will automatically handle backlight key presses.
> > Could this functionality be duplicated across other backlight drivers?
> > Not easily. The ACPI driver receives keypresses and performs backlight
> > control. The i915 driver doesn't receive keypresses. We could easily tie
> > certain keycodes into backlight events, but which backlight should they
> > control? You're really starting to get into the kind of complex policy
> > decision that's best left to userspace, which is where it should have
> > been to begin with.
> >
> Well, I get the reasoning, but I'm not sure I agree. That means
> userspace behavior is inconsistent depending on who does it
> (gnome-power-manager, gnome-setting-daemon, whatever), and it usually
> means there's nothing handling the brightness before those are running,
> not to mention people not running them because they don't run a full
> blown desktop environment (until someone starts thinking it's a good
> idea to handle brightness in systemd).
The behaviour is already inconsistent. If you have an ACPI backlight
interface, hitting the keys makes your brightness change without any
userspace help. If you don't, it doesn't.
> And in the end, the user just want the brightness keys to correctly
> handle the brightness, full stop. Having multiple brightness daemons
> using different policies on different hardware is a nightmare for the
> end user, imho. From a user point of view, having it handled all in the
> kernel works really pretty fine and is completely transparent (I have to
> admit that from that point of view, it was even better when it was
> handled by the EC but those times seem long gone).
I agree, we should standardise the behaviour. And the only way we can
standardise the behaviour is to leave it up to userspace.
--
Matthew Garrett | [email protected]
On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
> > On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> > > Right, the kernel has special-casing to hook the backlight keys up to
> > > the ACPI backlight control. This is an awful thing, because there's no
> > > way to detect this case other than parsing a single driver-specific
> > > module parameter.
> >
> > I'm not sure what that means. To detect what case exactly? That the
> > brightness is handled by video.ko?
>
> That the kernel will automatically handle backlight key presses.
Ok, so for detection by userspace? hal managed to do that just fine, it
seems that upower doesn't, for some reason.
> The behaviour is already inconsistent. If you have an ACPI backlight
> interface, hitting the keys makes your brightness change without any
> userspace help. If you don't, it doesn't.
At least on the same (class of) hardware it always behaves the same.
>
> > And in the end, the user just want the brightness keys to correctly
> > handle the brightness, full stop. Having multiple brightness daemons
> > using different policies on different hardware is a nightmare for the
> > end user, imho. From a user point of view, having it handled all in the
> > kernel works really pretty fine and is completely transparent (I have to
> > admit that from that point of view, it was even better when it was
> > handled by the EC but those times seem long gone).
>
> I agree, we should standardise the behaviour. And the only way we can
> standardise the behaviour is to leave it up to userspace.
>
It's pretty clear we disagree on this and that my opinion won't really
matter here. But letting userspace handle that just means broken
functionality for those who have the chance (apparently) to have an ACPI
backlight interface.
Regards,
--
Yves-Alexis
On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> > I agree, we should standardise the behaviour. And the only way we can
> > standardise the behaviour is to leave it up to userspace.
> >
> It's pretty clear we disagree on this and that my opinion won't really
> matter here. But letting userspace handle that just means broken
> functionality for those who have the chance (apparently) to have an ACPI
> backlight interface.
Which, as we've already established, you don't - Lenovo broke it. Your
Thinkpad claims to have 100 available levels, and most of them don't
work. The kernel has no way of knowing which levels work and which
don't, so leaving this up to the kernel won't actually fix your system
either.
--
Matthew Garrett | [email protected]
On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
> On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
> > On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> > > I agree, we should standardise the behaviour. And the only way we can
> > > standardise the behaviour is to leave it up to userspace.
> > >
> > It's pretty clear we disagree on this and that my opinion won't really
> > matter here. But letting userspace handle that just means broken
> > functionality for those who have the chance (apparently) to have an ACPI
> > backlight interface.
>
> Which, as we've already established, you don't - Lenovo broke it. Your
> Thinkpad claims to have 100 available levels, and most of them don't
> work. The kernel has no way of knowing which levels work and which
> don't, so leaving this up to the kernel won't actually fix your system
> either.
I was referring to “standardize the behaviour by leaving up to
userspace”. A lot of thinkpads (for example) (all the pre-windows 8
ones) have a perfectly working ACPI backlight interface.
Also, if the kernel has no way of knowing which levels work, I fail to
see how userspace can do better.
I understand that switching to intel_backlight instead of acpi_video0
follows what Windows 8 recommends but for me it looks orthogonal to the
fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
mean, it's not the first time firmware people break some kernel
behavior. I know it's usually not easy to contact them, but shouldn't
those methods be fixed, instead of somehow blindly switching to graphic
drivers?
--
Yves-Alexis
On Tue, Jun 25, 2013 at 11:30:37PM +0200, Yves-Alexis Perez wrote:
> On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
> > Which, as we've already established, you don't - Lenovo broke it. Your
> > Thinkpad claims to have 100 available levels, and most of them don't
> > work. The kernel has no way of knowing which levels work and which
> > don't, so leaving this up to the kernel won't actually fix your system
> > either.
>
> I was referring to “standardize the behaviour by leaving up to
> userspace”. A lot of thinkpads (for example) (all the pre-windows 8
> ones) have a perfectly working ACPI backlight interface.
And this patchset won't alter their behaviour.
> Also, if the kernel has no way of knowing which levels work, I fail to
> see how userspace can do better.
It can't. That's why this patchset disables the ACPI interface on
Windows 8 systems.
> I understand that switching to intel_backlight instead of acpi_video0
> follows what Windows 8 recommends but for me it looks orthogonal to the
> fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
> mean, it's not the first time firmware people break some kernel
> behavior. I know it's usually not easy to contact them, but shouldn't
> those methods be fixed, instead of somehow blindly switching to graphic
> drivers?
No. The correct answer to all firmware issues is "Are we making the same
firmware calls as the version of Windows that this hardware thinks it's
running". If Windows 8 doesn't make these calls, we shouldn't make these
calls.
--
Matthew Garrett | [email protected]
On mar., 2013-06-25 at 22:33 +0100, Matthew Garrett wrote:
> > I was referring to “standardize the behaviour by leaving up to
> > userspace”. A lot of thinkpads (for example) (all the pre-windows 8
> > ones) have a perfectly working ACPI backlight interface.
>
> And this patchset won't alter their behaviour.
Sorry if I was unclear and if my mail implied that. It was about your
remark later in the thread (and the mail from Daniel Vetter)
>
> > Also, if the kernel has no way of knowing which levels work, I fail to
> > see how userspace can do better.
>
> It can't. That's why this patchset disables the ACPI interface on
> Windows 8 systems.
>
> > I understand that switching to intel_backlight instead of acpi_video0
> > follows what Windows 8 recommends but for me it looks orthogonal to the
> > fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
> > mean, it's not the first time firmware people break some kernel
> > behavior. I know it's usually not easy to contact them, but shouldn't
> > those methods be fixed, instead of somehow blindly switching to graphic
> > drivers?
>
> No. The correct answer to all firmware issues is "Are we making the same
> firmware calls as the version of Windows that this hardware thinks it's
> running". If Windows 8 doesn't make these calls, we shouldn't make these
> calls.
But if that introduce regressions, shouldn't workarounds be found then?
Sorry if I keep repeating that but brightness keys handling in-kernel is
quite a useful feature and losing it (because of the “behave exactly
like Windows 8 kernel” policy) is indeed a regression.
--
Yves-Alexis
On Tue, Jun 25, 2013 at 11:46:01PM +0200, Yves-Alexis Perez wrote:
> But if that introduce regressions, shouldn't workarounds be found then?
> Sorry if I keep repeating that but brightness keys handling in-kernel is
> quite a useful feature and losing it (because of the “behave exactly
> like Windows 8 kernel” policy) is indeed a regression.
Your firmware behaves differently depending on whether the OS claims to
be Windows 8 or not. We can't make that invisible.
--
Matthew Garrett | [email protected]
On Tuesday, June 18, 2013 02:42:13 AM Rafael J. Wysocki wrote:
> On Monday, June 17, 2013 10:37:10 PM Matthew Garrett wrote:
> > On Tue, 2013-06-18 at 00:31 +0200, Rafael J. Wysocki wrote:
> > > Hi Matthew,
> > >
> > > On Sunday, June 09, 2013 07:01:38 PM Matthew Garrett wrote:
> > > > Drivers may need to make policy decisions based on the OS that the firmware
> > > > believes it's interacting with. ACPI firmware will make a series of _OSI
> > > > calls, starting from the oldest OS version they support and ending with the
> > > > most recent. Add a function to return the last successful call so that
> > > > drivers know what the firmware's expecting.
> > > >
> > > > Based on a patch by Seth Forshee <[email protected]>
> > >
> > > Bob (CCed) would prefer us to access acpi_gbl_osi_data directly instead of
> > > adding the wrapper to ACPICA. He also thinks that the symbol definitions
> > > should go into include/acpi/actypes.h rather than into acpixf.h.
> > >
> > > Then, the only ACPICA change would be to move the symbols and we can
> > > add a Linux-specific patch on top of that adding the acpi_gbl_osi_data
> > > wrapper.
> >
> > That sounds good to me. Do you want to respin that, or should I send an
> > updated set?
>
> That one should be fine, no need to respin [1/3] and [3/3].
Any news?
Rafael
This is a re-spin of patch 2/3:
ACPICA: Add interface for getting latest OS version requested via _OSI
to address the comments suggested by Bob and Rafael.
To be exact, the patchset does:
1 Expose acpi_gbl_osi_data through include/acpi/acpixf.h and move all
OSI definitions to include/acpi/actypes.h in patch 1;
2 Add a wrapper function acpi_osi_version in osl.c to return
acpi_gbl_osi_data in patch 2.
Matthew,
I suppose you might be busy so I've done the re-spin for you,
please let me know if you think something wrong, thanks.
Aaron Lu (2):
ACPICA: expose OSI version
ACPI / OSL: add a wrapper function to return OSI version
drivers/acpi/acpica/aclocal.h | 13 -------------
drivers/acpi/osl.c | 6 ++++++
include/acpi/acpixf.h | 1 +
include/acpi/actypes.h | 15 +++++++++++++++
include/linux/acpi.h | 6 ++++++
5 files changed, 28 insertions(+), 13 deletions(-)
--
1.8.3.2.10.g43d11f4
Expose acpi_gbl_osi_data so that driver can know the value of the last
successfull call of _OSI. The definitions for OSI versions have been
moved to actypes.h so that other components can access them.
Based on a patch by Matthew Garrett <[email protected]>, which
is again based on a patch by Seth Forshee <[email protected]>.
Signed-off-by: Aaron Lu <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Seth Forshee <[email protected]>
---
drivers/acpi/acpica/aclocal.h | 13 -------------
include/acpi/acpixf.h | 1 +
include/acpi/actypes.h | 15 +++++++++++++++
3 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index dfed265..d4a49016 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -931,19 +931,6 @@ struct acpi_bit_register_info {
/* Structs and definitions for _OSI support and I/O port validation */
-#define ACPI_OSI_WIN_2000 0x01
-#define ACPI_OSI_WIN_XP 0x02
-#define ACPI_OSI_WIN_XP_SP1 0x03
-#define ACPI_OSI_WINSRV_2003 0x04
-#define ACPI_OSI_WIN_XP_SP2 0x05
-#define ACPI_OSI_WINSRV_2003_SP1 0x06
-#define ACPI_OSI_WIN_VISTA 0x07
-#define ACPI_OSI_WINSRV_2008 0x08
-#define ACPI_OSI_WIN_VISTA_SP1 0x09
-#define ACPI_OSI_WIN_VISTA_SP2 0x0A
-#define ACPI_OSI_WIN_7 0x0B
-#define ACPI_OSI_WIN_8 0x0C
-
#define ACPI_ALWAYS_ILLEGAL 0x00
struct acpi_interface_info {
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 1b09300..22d497e 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -62,6 +62,7 @@ extern u32 acpi_current_gpe_count;
extern struct acpi_table_fadt acpi_gbl_FADT;
extern u8 acpi_gbl_system_awake_and_running;
extern u8 acpi_gbl_reduced_hardware; /* ACPI 5.0 */
+extern u8 acpi_gbl_osi_data;
/* Runtime configuration of debug print levels */
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index a64adcc..22b03c9 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -1144,4 +1144,19 @@ struct acpi_memory_list {
#endif
};
+/* Definitions for _OSI support */
+
+#define ACPI_OSI_WIN_2000 0x01
+#define ACPI_OSI_WIN_XP 0x02
+#define ACPI_OSI_WIN_XP_SP1 0x03
+#define ACPI_OSI_WINSRV_2003 0x04
+#define ACPI_OSI_WIN_XP_SP2 0x05
+#define ACPI_OSI_WINSRV_2003_SP1 0x06
+#define ACPI_OSI_WIN_VISTA 0x07
+#define ACPI_OSI_WINSRV_2008 0x08
+#define ACPI_OSI_WIN_VISTA_SP1 0x09
+#define ACPI_OSI_WIN_VISTA_SP2 0x0A
+#define ACPI_OSI_WIN_7 0x0B
+#define ACPI_OSI_WIN_8 0x0C
+
#endif /* __ACTYPES_H__ */
--
1.8.3.2.10.g43d11f4
Drivers may need to make policy decisions based on the OS that the firmware
believes it's interacting with. ACPI firmware will make a series of _OSI
calls, starting from the oldest OS version they support and ending with the
most recent. This patchset adds a function in ACPI OSL layer to return the
last successful call so that drivers know what the firmware's expecting.
Based on a patch by Matthew Garrett <[email protected]>, which
is again based on a patch by Seth Forshee <[email protected]>.
Changelog-by: Matthew Garrett <[email protected]>
Signed-off-by: Aaron Lu <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Seth Forshee <[email protected]>
---
drivers/acpi/osl.c | 6 ++++++
include/linux/acpi.h | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..7ebf07d 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1799,3 +1799,9 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
kfree(hp_work);
}
EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
+
+u8 acpi_osi_version(void)
+{
+ return acpi_gbl_osi_data;
+}
+EXPORT_SYMBOL_GPL(acpi_osi_version);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 353ba25..fc3b5c2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -578,4 +578,10 @@ acpi_handle_printk(const char *level, void *handle, const char *fmt, ...) {}
})
#endif
+#ifdef CONFIG_ACPI
+u8 acpi_osi_version(void);
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
#endif /*_LINUX_ACPI_H*/
--
1.8.3.2.10.g43d11f4
On Tuesday, July 02, 2013 10:01:27 PM Aaron Lu wrote:
> Drivers may need to make policy decisions based on the OS that the firmware
> believes it's interacting with. ACPI firmware will make a series of _OSI
> calls, starting from the oldest OS version they support and ending with the
> most recent. This patchset adds a function in ACPI OSL layer to return the
> last successful call so that drivers know what the firmware's expecting.
>
> Based on a patch by Matthew Garrett <[email protected]>, which
> is again based on a patch by Seth Forshee <[email protected]>.
>
> Changelog-by: Matthew Garrett <[email protected]>
> Signed-off-by: Aaron Lu <[email protected]>
> Cc: Matthew Garrett <[email protected]>
> Cc: Seth Forshee <[email protected]>
> ---
> drivers/acpi/osl.c | 6 ++++++
> include/linux/acpi.h | 6 ++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 6ab2c35..7ebf07d 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1799,3 +1799,9 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
> kfree(hp_work);
> }
> EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
> +
> +u8 acpi_osi_version(void)
> +{
> + return acpi_gbl_osi_data;
> +}
Actually, is there a reason not to make this static inline and put it into
the header?
Rafael
> +EXPORT_SYMBOL_GPL(acpi_osi_version);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 353ba25..fc3b5c2 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -578,4 +578,10 @@ acpi_handle_printk(const char *level, void *handle, const char *fmt, ...) {}
> })
> #endif
>
> +#ifdef CONFIG_ACPI
> +u8 acpi_osi_version(void);
> +#else
> +static inline u8 acpi_osi_version(void) { return 0; }
> +#endif
> +
> #endif /*_LINUX_ACPI_H*/
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 07/04/2013 05:57 AM, Rafael J. Wysocki wrote:
> On Tuesday, July 02, 2013 10:01:27 PM Aaron Lu wrote:
>> Drivers may need to make policy decisions based on the OS that the firmware
>> believes it's interacting with. ACPI firmware will make a series of _OSI
>> calls, starting from the oldest OS version they support and ending with the
>> most recent. This patchset adds a function in ACPI OSL layer to return the
>> last successful call so that drivers know what the firmware's expecting.
>>
>> Based on a patch by Matthew Garrett <[email protected]>, which
>> is again based on a patch by Seth Forshee <[email protected]>.
>>
>> Changelog-by: Matthew Garrett <[email protected]>
>> Signed-off-by: Aaron Lu <[email protected]>
>> Cc: Matthew Garrett <[email protected]>
>> Cc: Seth Forshee <[email protected]>
>> ---
>> drivers/acpi/osl.c | 6 ++++++
>> include/linux/acpi.h | 6 ++++++
>> 2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 6ab2c35..7ebf07d 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -1799,3 +1799,9 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
>> kfree(hp_work);
>> }
>> EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
>> +
>> +u8 acpi_osi_version(void)
>> +{
>> + return acpi_gbl_osi_data;
>> +}
>
> Actually, is there a reason not to make this static inline and put it into
> the header?
Ah right, thanks for the suggestion.
Patch updated:
From: Aaron Lu <[email protected]>
Subject: [PATCH updated 2/2] ACPI / OSL: add a wrapper function to return OSI version
Drivers may need to make policy decisions based on the OS that the firmware
believes it's interacting with. ACPI firmware will make a series of _OSI
calls, starting from the oldest OS version they support and ending with the
most recent. This patchset adds a function in ACPI OSL layer to return the
last successful call so that drivers know what the firmware's expecting.
Based on a patch by Matthew Garrett <[email protected]>, which
is again based on a patch by Seth Forshee <[email protected]>.
Signed-off-by: Aaron Lu <[email protected]>
---
include/linux/acpi.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 353ba25..7969bf8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -578,4 +578,10 @@ acpi_handle_printk(const char *level, void *handle, const char *fmt, ...) {}
})
#endif
+#ifdef CONFIG_ACPI
+static inline u8 acpi_osi_version(void) { return acpi_gbl_osi_data; }
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
#endif /*_LINUX_ACPI_H*/
--
1.8.3.2.10.g43d11f4
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
> 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 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.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 3b315ba..23b6292 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> /* Must be done after probing outputs */
> intel_opregion_init(dev);
> acpi_video_register();
> + /* Don't use ACPI backlight functions on Windows 8 platforms */
> + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> + acpi_video_backlight_unregister();
> }
>
> if (IS_GEN5(dev))
>
Well, this causes build failures to happen when the ACPI video driver is
modular and the graphics driver is not.
I'm not sure how to resolve that, so suggestions are welcome.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Thursday, July 04, 2013 09:24:03 AM Aaron Lu wrote:
> On 07/04/2013 05:57 AM, Rafael J. Wysocki wrote:
> > On Tuesday, July 02, 2013 10:01:27 PM Aaron Lu wrote:
> >> Drivers may need to make policy decisions based on the OS that the firmware
> >> believes it's interacting with. ACPI firmware will make a series of _OSI
> >> calls, starting from the oldest OS version they support and ending with the
> >> most recent. This patchset adds a function in ACPI OSL layer to return the
> >> last successful call so that drivers know what the firmware's expecting.
> >>
> >> Based on a patch by Matthew Garrett <[email protected]>, which
> >> is again based on a patch by Seth Forshee <[email protected]>.
> >>
> >> Changelog-by: Matthew Garrett <[email protected]>
> >> Signed-off-by: Aaron Lu <[email protected]>
> >> Cc: Matthew Garrett <[email protected]>
> >> Cc: Seth Forshee <[email protected]>
> >> ---
> >> drivers/acpi/osl.c | 6 ++++++
> >> include/linux/acpi.h | 6 ++++++
> >> 2 files changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> >> index 6ab2c35..7ebf07d 100644
> >> --- a/drivers/acpi/osl.c
> >> +++ b/drivers/acpi/osl.c
> >> @@ -1799,3 +1799,9 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
> >> kfree(hp_work);
> >> }
> >> EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
> >> +
> >> +u8 acpi_osi_version(void)
> >> +{
> >> + return acpi_gbl_osi_data;
> >> +}
> >
> > Actually, is there a reason not to make this static inline and put it into
> > the header?
>
> Ah right, thanks for the suggestion.
Well, I actually took the previous version, because that one exported the
symbol which was needed by modular graphics drivers.
That said I'm not sure if the whole design of this patchset is correct,
because for example ACPI_OSI_WIN_8 doesn't make sense for !CONFIG_ACPI.
Thanks,
Rafael
> Patch updated:
>
> From: Aaron Lu <[email protected]>
> Subject: [PATCH updated 2/2] ACPI / OSL: add a wrapper function to return OSI version
>
> Drivers may need to make policy decisions based on the OS that the firmware
> believes it's interacting with. ACPI firmware will make a series of _OSI
> calls, starting from the oldest OS version they support and ending with the
> most recent. This patchset adds a function in ACPI OSL layer to return the
> last successful call so that drivers know what the firmware's expecting.
>
> Based on a patch by Matthew Garrett <[email protected]>, which
> is again based on a patch by Seth Forshee <[email protected]>.
>
> Signed-off-by: Aaron Lu <[email protected]>
> ---
> include/linux/acpi.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 353ba25..7969bf8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -578,4 +578,10 @@ acpi_handle_printk(const char *level, void *handle, const char *fmt, ...) {}
> })
> #endif
>
> +#ifdef CONFIG_ACPI
> +static inline u8 acpi_osi_version(void) { return acpi_gbl_osi_data; }
> +#else
> +static inline u8 acpi_osi_version(void) { return 0; }
> +#endif
> +
> #endif /*_LINUX_ACPI_H*/
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
> On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
> > 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 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.
> >
> > Signed-off-by: Matthew Garrett <[email protected]>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 3b315ba..23b6292 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > /* Must be done after probing outputs */
> > intel_opregion_init(dev);
> > acpi_video_register();
> > + /* Don't use ACPI backlight functions on Windows 8 platforms */
> > + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> > + acpi_video_backlight_unregister();
> > }
> >
> > if (IS_GEN5(dev))
> >
>
> Well, this causes build failures to happen when the ACPI video driver is
> modular and the graphics driver is not.
>
> I'm not sure how to resolve that, so suggestions are welcome.
Actually, that happened with the radeon patch.
That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for
example.
What about making acpi_video_register() do the quirk instead? We could add an
argument to it indicating whether or not quirks should be applied.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
> On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
> > On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
> > > 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 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.
> > >
> > > Signed-off-by: Matthew Garrett <[email protected]>
> > > ---
> > > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 3b315ba..23b6292 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > > /* Must be done after probing outputs */
> > > intel_opregion_init(dev);
> > > acpi_video_register();
> > > + /* Don't use ACPI backlight functions on Windows 8 platforms */
> > > + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> > > + acpi_video_backlight_unregister();
> > > }
> > >
> > > if (IS_GEN5(dev))
> > >
> >
> > Well, this causes build failures to happen when the ACPI video driver is
> > modular and the graphics driver is not.
> >
> > I'm not sure how to resolve that, so suggestions are welcome.
>
> Actually, that happened with the radeon patch.
>
> That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for
> example.
>
> What about making acpi_video_register() do the quirk instead? We could add an
> argument to it indicating whether or not quirks should be applied.
Actually, I wonder what about the appended patch (on top of the Aaron's
https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.
Thanks,
Rafael
---
drivers/acpi/video_detect.c | 11 +++++++++--
include/linux/acpi.h | 1 +
2 files changed, 10 insertions(+), 2 deletions(-)
Index: linux-pm/drivers/acpi/video_detect.c
===================================================================
--- linux-pm.orig/drivers/acpi/video_detect.c
+++ linux-pm/drivers/acpi/video_detect.c
@@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
*/
dmi_check_system(video_detect_dmi_table);
+
+ if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
+ acpi_video_support |= ACPI_VIDEO_FORCE_NO_BACKLIGHT;
} else {
status = acpi_bus_get_device(graphics_handle, &tmp_dev);
if (ACPI_FAILURE(status)) {
@@ -258,13 +261,17 @@ int acpi_video_backlight_support(void)
{
acpi_video_caps_check();
- /* First check for boot param -> highest prio */
+ /* First, check if no backlight support has been forced upon us. */
+ if (acpi_video_support & ACPI_VIDEO_FORCE_NO_BACKLIGHT)
+ return 0;
+
+ /* Next check for boot param -> second highest prio */
if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
return 0;
else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO)
return 1;
- /* Then check for DMI blacklist -> second highest prio */
+ /* Then check for DMI blacklist -> third highest prio */
if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR)
return 0;
else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO)
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_FORCE_NO_BACKLIGHT 0x1000
#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
> On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
> > On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
> > > On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
> > > > 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 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.
> > > >
> > > > Signed-off-by: Matthew Garrett <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_dma.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > index 3b315ba..23b6292 100644
> > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > > > /* Must be done after probing outputs */
> > > > intel_opregion_init(dev);
> > > > acpi_video_register();
> > > > + /* Don't use ACPI backlight functions on Windows 8 platforms */
> > > > + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> > > > + acpi_video_backlight_unregister();
> > > > }
> > > >
> > > > if (IS_GEN5(dev))
> > > >
> > >
> > > Well, this causes build failures to happen when the ACPI video driver is
> > > modular and the graphics driver is not.
> > >
> > > I'm not sure how to resolve that, so suggestions are welcome.
> >
> > Actually, that happened with the radeon patch.
> >
> > That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for
> > example.
> >
> > What about making acpi_video_register() do the quirk instead? We could add an
> > argument to it indicating whether or not quirks should be applied.
>
> Actually, I wonder what about the appended patch (on top of the Aaron's
> https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.
Or even something as simple as this one.
---
drivers/acpi/video_detect.c | 3 +++
1 file changed, 3 insertions(+)
Index: linux-pm/drivers/acpi/video_detect.c
===================================================================
--- linux-pm.orig/drivers/acpi/video_detect.c
+++ linux-pm/drivers/acpi/video_detect.c
@@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
*/
dmi_check_system(video_detect_dmi_table);
+
+ if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
+ acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
} else {
status = acpi_bus_get_device(graphics_handle, &tmp_dev);
if (ACPI_FAILURE(status)) {
On 07/06/2013 06:23 AM, Rafael J. Wysocki wrote:
> On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
>> On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
>>> On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
>>>> On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
>>>>> 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 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.
>>>>>
>>>>> Signed-off-by: Matthew Garrett <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_dma.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>>>> index 3b315ba..23b6292 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>>>> @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>>>> /* Must be done after probing outputs */
>>>>> intel_opregion_init(dev);
>>>>> acpi_video_register();
>>>>> + /* Don't use ACPI backlight functions on Windows 8 platforms */
>>>>> + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
>>>>> + acpi_video_backlight_unregister();
>>>>> }
>>>>>
>>>>> if (IS_GEN5(dev))
>>>>>
>>>>
>>>> Well, this causes build failures to happen when the ACPI video driver is
>>>> modular and the graphics driver is not.
>>>>
>>>> I'm not sure how to resolve that, so suggestions are welcome.
>>>
>>> Actually, that happened with the radeon patch.
>>>
>>> That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for
>>> example.
>>>
>>> What about making acpi_video_register() do the quirk instead? We could add an
>>> argument to it indicating whether or not quirks should be applied.
>>
>> Actually, I wonder what about the appended patch (on top of the Aaron's
>> https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.
>
> Or even something as simple as this one.
>
> ---
> drivers/acpi/video_detect.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> Index: linux-pm/drivers/acpi/video_detect.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/video_detect.c
> +++ linux-pm/drivers/acpi/video_detect.c
> @@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
> */
>
> dmi_check_system(video_detect_dmi_table);
> +
> + if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
> + acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
Then vendor driver(thinkpad_acpi) will step in and create a backlight
interface for the system, which, unfortunately, is also broken for those
win8 thinkpads.
So we will need to do something in thinkpad_acpi to also not create
backlight interface for these systems too.
This actually doesn't feel bad to me, since the modules are blacklisting
their own interfaces. The downside is of course, two quirk code exist.
BTW, unregistering ACPI video's backlight interface in GPU driver doesn't
have this problem since it made the platform driver think the ACPI video
driver will control the backlight and then use the newly added API to
remove ACPI video created backlight interface.
Thanks,
Aaron
> } else {
> status = acpi_bus_get_device(graphics_handle, &tmp_dev);
> if (ACPI_FAILURE(status)) {
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Saturday, July 06, 2013 01:45:56 PM Aaron Lu wrote:
> On 07/06/2013 06:23 AM, Rafael J. Wysocki wrote:
> > On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
> >> On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
> >>> On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
> >>>> On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
> >>>>> 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 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.
> >>>>>
> >>>>> Signed-off-by: Matthew Garrett <[email protected]>
> >>>>> ---
> >>>>> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> >>>>> 1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >>>>> index 3b315ba..23b6292 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_dma.c
> >>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
> >>>>> @@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>>>> /* Must be done after probing outputs */
> >>>>> intel_opregion_init(dev);
> >>>>> acpi_video_register();
> >>>>> + /* Don't use ACPI backlight functions on Windows 8 platforms */
> >>>>> + if (acpi_osi_version() >= ACPI_OSI_WIN_8)
> >>>>> + acpi_video_backlight_unregister();
> >>>>> }
> >>>>>
> >>>>> if (IS_GEN5(dev))
> >>>>>
> >>>>
> >>>> Well, this causes build failures to happen when the ACPI video driver is
> >>>> modular and the graphics driver is not.
> >>>>
> >>>> I'm not sure how to resolve that, so suggestions are welcome.
> >>>
> >>> Actually, that happened with the radeon patch.
> >>>
> >>> That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for
> >>> example.
> >>>
> >>> What about making acpi_video_register() do the quirk instead? We could add an
> >>> argument to it indicating whether or not quirks should be applied.
> >>
> >> Actually, I wonder what about the appended patch (on top of the Aaron's
> >> https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.
> >
> > Or even something as simple as this one.
> >
> > ---
> > drivers/acpi/video_detect.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > Index: linux-pm/drivers/acpi/video_detect.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/video_detect.c
> > +++ linux-pm/drivers/acpi/video_detect.c
> > @@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
> > */
> >
> > dmi_check_system(video_detect_dmi_table);
> > +
> > + if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
> > + acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
>
> Then vendor driver(thinkpad_acpi) will step in and create a backlight
> interface for the system, which, unfortunately, is also broken for those
> win8 thinkpads.
>
> So we will need to do something in thinkpad_acpi to also not create
> backlight interface for these systems too.
>
> This actually doesn't feel bad to me, since the modules are blacklisting
> their own interfaces. The downside is of course, two quirk code exist.
>
> BTW, unregistering ACPI video's backlight interface in GPU driver doesn't
> have this problem since it made the platform driver think the ACPI video
> driver will control the backlight and then use the newly added API to
> remove ACPI video created backlight interface.
Yes, I know this.
I think I also know how to introduce that change in a slightly cleaner way.
I'll post a patch for comments later today or tomorrow.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Saturday, July 06, 2013 03:33:01 PM Rafael J. Wysocki wrote:
> On Saturday, July 06, 2013 01:45:56 PM Aaron Lu wrote:
> > On 07/06/2013 06:23 AM, Rafael J. Wysocki wrote:
> > > On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
> > >> On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
> > >>> On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
> > >>>> On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
[...]
> > >>
> > >> Actually, I wonder what about
[...]
> > >
> > > Or even something as simple as this one.
> > >
> > > ---
> > > drivers/acpi/video_detect.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > Index: linux-pm/drivers/acpi/video_detect.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/video_detect.c
> > > +++ linux-pm/drivers/acpi/video_detect.c
> > > @@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
> > > */
> > >
> > > dmi_check_system(video_detect_dmi_table);
> > > +
> > > + if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
> > > + acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
> >
> > Then vendor driver(thinkpad_acpi) will step in and create a backlight
> > interface for the system, which, unfortunately, is also broken for those
> > win8 thinkpads.
> >
> > So we will need to do something in thinkpad_acpi to also not create
> > backlight interface for these systems too.
> >
> > This actually doesn't feel bad to me, since the modules are blacklisting
> > their own interfaces. The downside is of course, two quirk code exist.
> >
> > BTW, unregistering ACPI video's backlight interface in GPU driver doesn't
> > have this problem since it made the platform driver think the ACPI video
> > driver will control the backlight and then use the newly added API to
> > remove ACPI video created backlight interface.
>
> Yes, I know this.
>
> I think I also know how to introduce that change in a slightly cleaner way.
>
> I'll post a patch for comments later today or tomorrow.
OK, the patch is appended. Please have a look and tell me what you think.
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8
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 the ACPI one is not
registered in advance.
Therefore we need to keep the ACPI backlight interface registered
until the i915 driver is loaded which then will unregister it if
the firmware has called _OSI for Windows 8.
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.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/video.c | 61 ++++++++++++++++++++++++++++++++++++----
drivers/acpi/video_detect.c | 11 +++++++
drivers/gpu/drm/i915/i915_dma.c | 2 -
include/acpi/video.h | 11 ++++++-
include/linux/acpi.h | 6 +++
5 files changed, 84 insertions(+), 7 deletions(-)
Index: linux-pm/drivers/acpi/video.c
===================================================================
--- linux-pm.orig/drivers/acpi/video.c
+++ linux-pm/drivers/acpi/video.c
@@ -1854,6 +1854,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 +1925,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 +1959,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
@@ -234,6 +234,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_BACKLIGHT_FORCE_VENDOR;
+ 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
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -196,6 +196,7 @@ extern bool wmi_has_guid(const char *gui
extern long acpi_video_get_capabilities(acpi_handle graphics_dev_handle);
extern long acpi_is_video_device(acpi_handle handle);
+extern bool acpi_video_backlight_quirks(void);
extern void acpi_video_dmi_promote_vendor(void);
extern void acpi_video_dmi_demote_vendor(void);
extern int acpi_video_backlight_support(void);
@@ -213,6 +214,11 @@ static inline long acpi_is_video_device(
return 0;
}
+static inline bool acpi_video_backlight_quirks(void)
+{
+ return false;
+}
+
static inline void acpi_video_dmi_promote_vendor(void)
{
}
On 07/07/2013 09:19 PM, Rafael J. Wysocki wrote:
> OK, the patch is appended. Please have a look and tell me what you think.
>
> Thanks,
> Rafael
>
>
> ---
> From: Rafael J. Wysocki <[email protected]>
> Subject: ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8
>
> 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 the ACPI one is not
> registered in advance.
The condition thinkpad_acpi checks to decide if it wants to create
backlight control interface is acpi_video_backlight_support function,
not that if ACPI video driver has registered previously. I'm sorry if my
previous words gave you such a conclusion.
> Therefore we need to keep the ACPI backlight interface registered
> until the i915 driver is loaded which then will unregister it if
> the firmware has called _OSI for Windows 8.
>
> 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.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/video.c | 61 ++++++++++++++++++++++++++++++++++++----
> drivers/acpi/video_detect.c | 11 +++++++
> drivers/gpu/drm/i915/i915_dma.c | 2 -
> include/acpi/video.h | 11 ++++++-
> include/linux/acpi.h | 6 +++
> 5 files changed, 84 insertions(+), 7 deletions(-)
>
> Index: linux-pm/drivers/acpi/video.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/video.c
> +++ linux-pm/drivers/acpi/video.c
> @@ -1854,6 +1854,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 +1925,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 +1959,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
> @@ -234,6 +234,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_BACKLIGHT_FORCE_VENDOR;
If the ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR flag is set,
acpi_video_backlight_support will return 0, thinkpad_acpi will create
backlight interface for the system, which is not what we want.
Thanks,
Aaron
> + 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
> Index: linux-pm/include/linux/acpi.h
> ===================================================================
> --- linux-pm.orig/include/linux/acpi.h
> +++ linux-pm/include/linux/acpi.h
> @@ -196,6 +196,7 @@ extern bool wmi_has_guid(const char *gui
>
> extern long acpi_video_get_capabilities(acpi_handle graphics_dev_handle);
> extern long acpi_is_video_device(acpi_handle handle);
> +extern bool acpi_video_backlight_quirks(void);
> extern void acpi_video_dmi_promote_vendor(void);
> extern void acpi_video_dmi_demote_vendor(void);
> extern int acpi_video_backlight_support(void);
> @@ -213,6 +214,11 @@ static inline long acpi_is_video_device(
> return 0;
> }
>
> +static inline bool acpi_video_backlight_quirks(void)
> +{
> + return false;
> +}
> +
> static inline void acpi_video_dmi_promote_vendor(void)
> {
> }
>
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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_ */
On 07/13/2013 08:46 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Aaron Lu <[email protected]>
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.
-Aaron
> ---
> 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_ */
>
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 <[email protected]>
> >
> > 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 <[email protected]>
>
> Reviewed-by: Aaron Lu <[email protected]>
>
> 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?
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_ */
> >
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Sat, 2013-07-13 at 02:46 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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_ */
I can't build it. Where did I go wrong?
drivers/acpi/video_detect.c:239:6: error: redefinition of
'acpi_video_backlight_quirks'
bool acpi_video_backlight_quirks(void)
^
In file included from drivers/acpi/video_detect.c:41:0:
drivers/acpi/internal.h:174:60: note: previous definition of
'acpi_video_backlight_quirks' was here
static inline bool acpi_video_backlight_quirks(void) { return false; }
^
drivers/acpi/video_detect.c: In function 'acpi_video_backlight_quirks':
drivers/acpi/video_detect.c:241:6: error: 'acpi_gbl_osi_data' undeclared
(first use in this function)
if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
^
drivers/acpi/video_detect.c:241:6: note: each undeclared identifier is
reported only once for each function it appears in
drivers/acpi/video_detect.c:241:27: error: 'ACPI_OSI_WIN_8' undeclared
(first use in this function)
if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
^
drivers/acpi/video_detect.c: At top level:
drivers/acpi/video_detect.c:295:6: error: redefinition of
'acpi_video_verify_backlight_support'
bool acpi_video_verify_backlight_support(void)
^
In file included from drivers/acpi/video_detect.c:41:0:
drivers/acpi/internal.h:175:60: note: previous definition of
'acpi_video_verify_backlight_support' was here
static inline bool acpi_video_verify_backlight_support(void) { return
false; }
^
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.9.9-302.fc19.x86_64
On Monday, July 15, 2013 05:06:09 PM Igor Gnatenko wrote:
> On Sat, 2013-07-13 at 02:46 +0200, Rafael J. Wysocki wrote:
[...]
>
> I can't build it. Where did I go wrong?
Probably nowhere, you tried to build the ACPI video driver as a module, that's
all. And you need to apply https://patchwork.kernel.org/patch/2812951/
before.
Please try the appended version (on top of
https://patchwork.kernel.org/patch/2812951/).
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / video / i915: No ACPI backlight if firmware expects Windows 8
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 <[email protected]>
---
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
@@ -1648,7 +1648,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 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
+ -------------------------------------------------------------------------- */
+#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_ */
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>> Reviewed-by: Aaron Lu <[email protected]>
>>
>> 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_ */
>>>
>>
On Tue, 2013-07-16 at 01:53 +0200, Rafael J. Wysocki wrote:
> On Monday, July 15, 2013 05:06:09 PM Igor Gnatenko wrote:
> > On Sat, 2013-07-13 at 02:46 +0200, Rafael J. Wysocki wrote:
>
> [...]
>
> >
> > I can't build it. Where did I go wrong?
>
> Probably nowhere, you tried to build the ACPI video driver as a module, that's
> all. And you need to apply https://patchwork.kernel.org/patch/2812951/
> before.
>
> Please try the appended version (on top of
> https://patchwork.kernel.org/patch/2812951/).
>
> Thanks,
> Rafael
>
Tested-by: Igor Gnatenko <[email protected]>
>
> ---
> From: Rafael J. Wysocki <[email protected]>
> Subject: ACPI / video / i915: No ACPI backlight if firmware expects Windows 8
>
> 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 <[email protected]>
> ---
> 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
> @@ -1648,7 +1648,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 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
> + -------------------------------------------------------------------------- */
> +#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_ */
>
>
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.11.0-0.rc0.git7.1.fc20.x86_64
On Tuesday, July 16, 2013 11:24:05 AM Aaron Lu wrote:
> 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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>
> >> Reviewed-by: Aaron Lu <[email protected]>
> >>
> >> 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.
OK
Thanks,
Rafael
Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.11.0-0.rc0.git7.1.fc20.x86_64
On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
> Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
Yeah, I can duplicate that. Rafael, we have to call
acpi_video_init_brightness() even if we're not going to initialise the
backlight - Thinkpads seem to use this as the trigger for enabling ACPI
notifications rather than handling it in firmware. This seems to do the
job:
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 01b1a25..71865f7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
device->cap._DDC = 1;
}
+ if (acpi_video_init_brightness(device))
+ return;
+
if (acpi_video_verify_backlight_support()) {
struct backlight_properties props;
struct pci_dev *pdev;
@@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
static int count = 0;
char *name;
- result = acpi_video_init_brightness(device);
- if (result)
- return;
name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
if (!name)
return;
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
> On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
> > Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
>
> Yeah, I can duplicate that. Rafael, we have to call
> acpi_video_init_brightness() even if we're not going to initialise the
> backlight - Thinkpads seem to use this as the trigger for enabling ACPI
> notifications rather than handling it in firmware. This seems to do the
> job:
Igor, does this additional patch from Matthew help?
Rafael
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 01b1a25..71865f7 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct
> acpi_video_device *device)
> device->cap._DDC = 1;
> }
>
> + if (acpi_video_init_brightness(device))
> + return;
> +
> if (acpi_video_verify_backlight_support()) {
> struct backlight_properties props;
> struct pci_dev *pdev;
> @@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct
> acpi_video_device *device)
> static int count = 0;
> char *name;
>
> - result = acpi_video_init_brightness(device);
> - if (result)
> - return;
> name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
> if (!name)
> return;
>
>
> --
> Matthew Garrett | [email protected]
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, 2013-07-17 at 00:01 +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
> > On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
> > > Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
> >
> > Yeah, I can duplicate that. Rafael, we have to call
> > acpi_video_init_brightness() even if we're not going to initialise the
> > backlight - Thinkpads seem to use this as the trigger for enabling ACPI
> > notifications rather than handling it in firmware. This seems to do the
> > job:
>
> Igor, does this additional patch from Matthew help?
Yes. With this patch I have backlight switch indicator on my ThinkPad X230.
>
> Rafael
>
>
Tested-by: Igor Gnatenko <[email protected]>
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index 01b1a25..71865f7 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct
> > acpi_video_device *device)
> > device->cap._DDC = 1;
> > }
> >
> > + if (acpi_video_init_brightness(device))
> > + return;
> > +
> > if (acpi_video_verify_backlight_support()) {
> > struct backlight_properties props;
> > struct pci_dev *pdev;
> > @@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct
> > acpi_video_device *device)
> > static int count = 0;
> > char *name;
> >
> > - result = acpi_video_init_brightness(device);
> > - if (result)
> > - return;
> > name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
> > if (!name)
> > return;
> >
> >
> > --
> > Matthew Garrett | [email protected]
> >
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.11.0-0.rc0.git7.1.fc20.x86_64
On Wednesday, July 17, 2013 09:16:38 AM Igor Gnatenko wrote:
> On Wed, 2013-07-17 at 00:01 +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
> > > On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
> > > > Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
> > >
> > > Yeah, I can duplicate that. Rafael, we have to call
> > > acpi_video_init_brightness() even if we're not going to initialise the
> > > backlight - Thinkpads seem to use this as the trigger for enabling ACPI
> > > notifications rather than handling it in firmware. This seems to do the
> > > job:
> >
> > Igor, does this additional patch from Matthew help?
> Yes. With this patch I have backlight switch indicator on my ThinkPad X230.
OK, thanks for the confirmation.
Can you please also check if applying the appended patch on top of the Matthew's
one changes anything (ie. things still work)?
Rafael
---
drivers/acpi/video.c | 5 +++++
1 file changed, 5 insertions(+)
Index: linux-pm/drivers/acpi/video.c
===================================================================
--- linux-pm.orig/drivers/acpi/video.c
+++ linux-pm/drivers/acpi/video.c
@@ -957,6 +957,11 @@ static void acpi_video_device_find_cap(s
if (result)
printk(KERN_ERR PREFIX "Create sysfs link\n");
+ } else {
+ /* Remove the brightness object. */
+ kfree(device->brightness->levels);
+ kfree(device->brightness);
+ device->brightness = NULL;
}
}
On Wed, 2013-07-17 at 13:38 +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 17, 2013 09:16:38 AM Igor Gnatenko wrote:
> > On Wed, 2013-07-17 at 00:01 +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
> > > > On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
> > > > > Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
> > > >
> > > > Yeah, I can duplicate that. Rafael, we have to call
> > > > acpi_video_init_brightness() even if we're not going to initialise the
> > > > backlight - Thinkpads seem to use this as the trigger for enabling ACPI
> > > > notifications rather than handling it in firmware. This seems to do the
> > > > job:
> > >
> > > Igor, does this additional patch from Matthew help?
> > Yes. With this patch I have backlight switch indicator on my ThinkPad X230.
>
> OK, thanks for the confirmation.
>
> Can you please also check if applying the appended patch on top of the Matthew's
> one changes anything (ie. things still work)?
Yes. I've tested and not found regressions in indicator or in switcher. Good work.
>
> Rafael
>
>
> ---
Tested-by: Igor Gnatenko <[email protected]>
> drivers/acpi/video.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> Index: linux-pm/drivers/acpi/video.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/video.c
> +++ linux-pm/drivers/acpi/video.c
> @@ -957,6 +957,11 @@ static void acpi_video_device_find_cap(s
> if (result)
> printk(KERN_ERR PREFIX "Create sysfs link\n");
>
> + } else {
> + /* Remove the brightness object. */
> + kfree(device->brightness->levels);
> + kfree(device->brightness);
> + device->brightness = NULL;
> }
> }
>
>
>
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.11.0-0.rc0.git7.1.fc20.x86_64
On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez <[email protected]> wrote:
> On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote:
>> The first two patches in this series are picked from other patchesets aimed at
>> solving similar problems. The last simply unregisters ACPI backlight control
>> on Windows 8 systems when using an Intel GPU. Similar code could be added to
>> other drivers, but I'm reluctant to do so without further investigation as
>> to the behaviour of the vendor drivers under Windows.
>
> Hi,
>
> I've read this thread coming from
> https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches
> on a Lenovo ThinkPad X230 with intel graphics.
>
> The problem with thoses fixes is that they still introduce a regression
> in how the brightness is handled, at least for me.
For me too.
> Before Linux support for acpi_osi("Windows 2012") (and when booting with
> acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> just fine, whether in console, in the display manager or in my desktop
> environment (Xfce). xfce4-power-manager just needs to be told that the
> brightness keys are already handled and it doesn't need to do anything.
How do you tell xfce4-power-manager that?
For me everything works fine when acpi_osi="!Windows 2012", which is
why I wrote a patch for this particular laptop.
http://article.gmane.org/gmane.linux.acpi.devel/60969
> So can the previous behavior be actually restored?
It *should*. The #1 rule of the Linux kernel is to never ever break
user-space, isn't it?
> Please keep me on CC: on replies, I'm not subscribed to the various
> lists.
You don't need to ask that in mailing lists that don't have reply-to
munged (sane ones), and vger ones don't.
Cheers.
--
Felipe Contreras
On mer., 2013-07-17 at 10:51 -0500, Felipe Contreras wrote:
> On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez <[email protected]> wrote:
> > Before Linux support for acpi_osi("Windows 2012") (and when booting with
> > acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> > just fine, whether in console, in the display manager or in my desktop
> > environment (Xfce). xfce4-power-manager just needs to be told that the
> > brightness keys are already handled and it doesn't need to do anything.
>
> How do you tell xfce4-power-manager that?
xfconf-query -c xfce4-power-manager
-p /xfce4-power-manager/change-brightness-on-key-events -s false
You might have to create the key before. See
https://bugzilla.xfce.org/show_bug.cgi?id=7541 for more information.
Regards,
--
Yves-Alexis
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
> Windows 8 introduced new policy for backlight control by pushing it out to
> graphics drivers. This appears to have coincided with a range of vendors
> adding Windows 8 checks to their backlight control code which trigger either
> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
> thing to do would be to just disable ACPI backlight control entirely if the
> firmware indicates Windows 8 support, but it's entirely possible that
> individual graphics drivers might still make use of the ACPI functionality in
> preference to native control.
>
> The first two patches in this series are picked from other patchesets aimed at
> solving similar problems. The last simply unregisters ACPI backlight control
> on Windows 8 systems when using an Intel GPU. Similar code could be added to
> other drivers, but I'm reluctant to do so without further investigation as
> to the behaviour of the vendor drivers under Windows.
Well, after some more time spent on that, we now have a series of 3 patches
(different from the $subject one) that we think may be used to address this
issue. As far as I can say, it has been tested by multiple people whose
systems have those problems and they generally saw improvement.
It is not my ideal approach, but it seems to be the least intrusive and/or
with the least amount of possible side effects that we can do right now
as a general measure (alternatively, we could create a possibly long
blacklist table of affected systems with different workarounds for them,
but let's just say that is not overwhelmingly attractive).
[1/3] Make ACPICA export things that we need for checking OSI(Win8).
[2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even
if it is not going to register the backlight interface (needed for
Thinkpads).
[3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes
we are Windows 8.
Many thanks to everyone involved!
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
From: Aaron Lu <[email protected]>
Expose acpi_gbl_osi_data so that code outside of ACPICA can check
the value of the last successfull _OSI call. The definitions for
OSI versions are moved to actypes.h so that other components can
access them too.
Based on a patch from Matthew Garrett which in turn was based on
an earlier patch from Seth Forshee.
[rjw: Changelog]
Signed-off-by: Aaron Lu <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/acpica/aclocal.h | 13 -------------
include/acpi/acpixf.h | 1 +
include/acpi/actypes.h | 15 +++++++++++++++
3 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index dfed265..d4a49016 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -931,19 +931,6 @@ struct acpi_bit_register_info {
/* Structs and definitions for _OSI support and I/O port validation */
-#define ACPI_OSI_WIN_2000 0x01
-#define ACPI_OSI_WIN_XP 0x02
-#define ACPI_OSI_WIN_XP_SP1 0x03
-#define ACPI_OSI_WINSRV_2003 0x04
-#define ACPI_OSI_WIN_XP_SP2 0x05
-#define ACPI_OSI_WINSRV_2003_SP1 0x06
-#define ACPI_OSI_WIN_VISTA 0x07
-#define ACPI_OSI_WINSRV_2008 0x08
-#define ACPI_OSI_WIN_VISTA_SP1 0x09
-#define ACPI_OSI_WIN_VISTA_SP2 0x0A
-#define ACPI_OSI_WIN_7 0x0B
-#define ACPI_OSI_WIN_8 0x0C
-
#define ACPI_ALWAYS_ILLEGAL 0x00
struct acpi_interface_info {
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 1b09300..22d497e 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -62,6 +62,7 @@ extern u32 acpi_current_gpe_count;
extern struct acpi_table_fadt acpi_gbl_FADT;
extern u8 acpi_gbl_system_awake_and_running;
extern u8 acpi_gbl_reduced_hardware; /* ACPI 5.0 */
+extern u8 acpi_gbl_osi_data;
/* Runtime configuration of debug print levels */
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index a64adcc..22b03c9 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -1144,4 +1144,19 @@ struct acpi_memory_list {
#endif
};
+/* Definitions for _OSI support */
+
+#define ACPI_OSI_WIN_2000 0x01
+#define ACPI_OSI_WIN_XP 0x02
+#define ACPI_OSI_WIN_XP_SP1 0x03
+#define ACPI_OSI_WINSRV_2003 0x04
+#define ACPI_OSI_WIN_XP_SP2 0x05
+#define ACPI_OSI_WINSRV_2003_SP1 0x06
+#define ACPI_OSI_WIN_VISTA 0x07
+#define ACPI_OSI_WINSRV_2008 0x08
+#define ACPI_OSI_WIN_VISTA_SP1 0x09
+#define ACPI_OSI_WIN_VISTA_SP2 0x0A
+#define ACPI_OSI_WIN_7 0x0B
+#define ACPI_OSI_WIN_8 0x0C
+
#endif /* __ACTYPES_H__ */
From: Matthew Garrett <[email protected]>
We have to call acpi_video_init_brightness() even if we're not going
to initialise the backlight - Thinkpads seem to use this as the
trigger for enabling ACPI notifications rather than handling it in
firmware.
[rjw: Drop the brightness object created by
acpi_video_init_brightness() if we are not going to use it.]
Signed-off-by: Matthew Garrett <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/video.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Index: linux-pm/drivers/acpi/video.c
===================================================================
--- linux-pm.orig/drivers/acpi/video.c
+++ linux-pm/drivers/acpi/video.c
@@ -884,6 +884,9 @@ static void acpi_video_device_find_cap(s
if (acpi_has_method(device->dev->handle, "_DDC"))
device->cap._DDC = 1;
+ if (acpi_video_init_brightness(device))
+ return;
+
if (acpi_video_backlight_support()) {
struct backlight_properties props;
struct pci_dev *pdev;
@@ -893,9 +896,6 @@ static void acpi_video_device_find_cap(s
static int count = 0;
char *name;
- result = acpi_video_init_brightness(device);
- if (result)
- return;
name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
if (!name)
return;
@@ -955,6 +955,11 @@ static void acpi_video_device_find_cap(s
if (result)
printk(KERN_ERR PREFIX "Create sysfs link\n");
+ } else {
+ /* Remove the brightness object. */
+ kfree(device->brightness->levels);
+ kfree(device->brightness);
+ device->brightness = NULL;
}
}
From: Rafael J. Wysocki <[email protected]>
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 includes a fix from Aaron Lu's.
References: https://bugzilla.kernel.org/show_bug.cgi?id=51231
Tested-by: Aaron Lu <[email protected]>
Tested-by: Igor Gnatenko <[email protected]>
Tested-by: Yves-Alexis Perez <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Aaron Lu <[email protected]>
Acked-by: Matthew Garrett <[email protected]>
---
drivers/acpi/internal.h | 11 ++++++
drivers/acpi/video.c | 69 +++++++++++++++++++++++++++++++++++-----
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, 105 insertions(+), 10 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"
@@ -887,7 +889,7 @@ static void acpi_video_device_find_cap(s
if (acpi_video_init_brightness(device))
return;
- if (acpi_video_backlight_support()) {
+ if (acpi_video_verify_backlight_support()) {
struct backlight_properties props;
struct pci_dev *pdev;
acpi_handle acpi_parent;
@@ -1334,8 +1336,8 @@ acpi_video_switch_brightness(struct acpi
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)
@@ -1837,6 +1839,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)
@@ -1868,14 +1910,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;
}
@@ -1891,7 +1944,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
@@ -1648,7 +1648,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");
@@ -231,6 +233,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
@@ -275,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.
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
+ -------------------------------------------------------------------------- */
+#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_ */
From: Aaron Lu <[email protected]>
Expose acpi_gbl_osi_data so that code outside of ACPICA can check
the value of the last successfull _OSI call. The definitions for
OSI versions are moved to actypes.h so that other components can
access them too.
Based on a patch from Matthew Garrett which in turn was based on
an earlier patch from Seth Forshee.
[rjw: Changelog]
Signed-off-by: Aaron Lu <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Igor Gnatenko <[email protected]>
---
drivers/acpi/acpica/aclocal.h | 13 -------------
include/acpi/acpixf.h | 1 +
include/acpi/actypes.h | 15 +++++++++++++++
3 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index dfed265..d4a49016 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -931,19 +931,6 @@ struct acpi_bit_register_info {
/* Structs and definitions for _OSI support and I/O port validation */
-#define ACPI_OSI_WIN_2000 0x01
-#define ACPI_OSI_WIN_XP 0x02
-#define ACPI_OSI_WIN_XP_SP1 0x03
-#define ACPI_OSI_WINSRV_2003 0x04
-#define ACPI_OSI_WIN_XP_SP2 0x05
-#define ACPI_OSI_WINSRV_2003_SP1 0x06
-#define ACPI_OSI_WIN_VISTA 0x07
-#define ACPI_OSI_WINSRV_2008 0x08
-#define ACPI_OSI_WIN_VISTA_SP1 0x09
-#define ACPI_OSI_WIN_VISTA_SP2 0x0A
-#define ACPI_OSI_WIN_7 0x0B
-#define ACPI_OSI_WIN_8 0x0C
-
#define ACPI_ALWAYS_ILLEGAL 0x00
struct acpi_interface_info {
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 1b09300..22d497e 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -62,6 +62,7 @@ extern u32 acpi_current_gpe_count;
extern struct acpi_table_fadt acpi_gbl_FADT;
extern u8 acpi_gbl_system_awake_and_running;
extern u8 acpi_gbl_reduced_hardware; /* ACPI 5.0 */
+extern u8 acpi_gbl_osi_data;
/* Runtime configuration of debug print levels */
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index a64adcc..22b03c9 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -1144,4 +1144,19 @@ struct acpi_memory_list {
#endif
};
+/* Definitions for _OSI support */
+
+#define ACPI_OSI_WIN_2000 0x01
+#define ACPI_OSI_WIN_XP 0x02
+#define ACPI_OSI_WIN_XP_SP1 0x03
+#define ACPI_OSI_WINSRV_2003 0x04
+#define ACPI_OSI_WIN_XP_SP2 0x05
+#define ACPI_OSI_WINSRV_2003_SP1 0x06
+#define ACPI_OSI_WIN_VISTA 0x07
+#define ACPI_OSI_WINSRV_2008 0x08
+#define ACPI_OSI_WIN_VISTA_SP1 0x09
+#define ACPI_OSI_WIN_VISTA_SP2 0x0A
+#define ACPI_OSI_WIN_7 0x0B
+#define ACPI_OSI_WIN_8 0x0C
+
#endif /* __ACTYPES_H__ */
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.9.9-302.fc19.x86_64
From: Matthew Garrett <[email protected]>
We have to call acpi_video_init_brightness() even if we're not going
to initialise the backlight - Thinkpads seem to use this as the
trigger for enabling ACPI notifications rather than handling it in
firmware.
[rjw: Drop the brightness object created by
acpi_video_init_brightness() if we are not going to use it.]
Signed-off-by: Matthew Garrett <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Igor Gnatenko <[email protected]>
---
drivers/acpi/video.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Index: linux-pm/drivers/acpi/video.c
===================================================================
--- linux-pm.orig/drivers/acpi/video.c
+++ linux-pm/drivers/acpi/video.c
@@ -884,6 +884,9 @@ static void acpi_video_device_find_cap(s
if (acpi_has_method(device->dev->handle, "_DDC"))
device->cap._DDC = 1;
+ if (acpi_video_init_brightness(device))
+ return;
+
if (acpi_video_backlight_support()) {
struct backlight_properties props;
struct pci_dev *pdev;
@@ -893,9 +896,6 @@ static void acpi_video_device_find_cap(s
static int count = 0;
char *name;
- result = acpi_video_init_brightness(device);
- if (result)
- return;
name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
if (!name)
return;
@@ -955,6 +955,11 @@ static void acpi_video_device_find_cap(s
if (result)
printk(KERN_ERR PREFIX "Create sysfs link\n");
+ } else {
+ /* Remove the brightness object. */
+ kfree(device->brightness->levels);
+ kfree(device->brightness);
+ device->brightness = NULL;
}
}
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.9.9-302.fc19.x86_64
On Wed, Jul 17, 2013 at 7:16 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
>> Windows 8 introduced new policy for backlight control by pushing it out to
>> graphics drivers. This appears to have coincided with a range of vendors
>> adding Windows 8 checks to their backlight control code which trigger either
>> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
>> thing to do would be to just disable ACPI backlight control entirely if the
>> firmware indicates Windows 8 support, but it's entirely possible that
>> individual graphics drivers might still make use of the ACPI functionality in
>> preference to native control.
>>
>> The first two patches in this series are picked from other patchesets aimed at
>> solving similar problems. The last simply unregisters ACPI backlight control
>> on Windows 8 systems when using an Intel GPU. Similar code could be added to
>> other drivers, but I'm reluctant to do so without further investigation as
>> to the behaviour of the vendor drivers under Windows.
>
> Well, after some more time spent on that, we now have a series of 3 patches
> (different from the $subject one) that we think may be used to address this
> issue. As far as I can say, it has been tested by multiple people whose
> systems have those problems and they generally saw improvement.
>
> It is not my ideal approach, but it seems to be the least intrusive and/or
> with the least amount of possible side effects that we can do right now
> as a general measure (alternatively, we could create a possibly long
> blacklist table of affected systems with different workarounds for them,
> but let's just say that is not overwhelmingly attractive).
>
> [1/3] Make ACPICA export things that we need for checking OSI(Win8).
>
> [2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even
> if it is not going to register the backlight interface (needed for
> Thinkpads).
>
> [3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes
> we are Windows 8.
>
> Many thanks to everyone involved!
I tried this patch series and it's as I expected, it's the same as
acpi_backlight=vendor, and the intel backlight driver doesn't work
correctly in this machine. If you are actually serious about the
mantra of "no user-space regressions", then for this machine at least,
you need to use the ACPI backlight with Windows8 OSI disabled, until
the intel backlight driver is fixed. My patch does that:
http://article.gmane.org/gmane.linux.acpi.devel/60969
--
Felipe Contreras
On Thursday, July 18, 2013 02:16:09 AM Rafael J. Wysocki wrote:
> On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
> > Windows 8 introduced new policy for backlight control by pushing it out to
> > graphics drivers. This appears to have coincided with a range of vendors
> > adding Windows 8 checks to their backlight control code which trigger either
> > awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
> > thing to do would be to just disable ACPI backlight control entirely if the
> > firmware indicates Windows 8 support, but it's entirely possible that
> > individual graphics drivers might still make use of the ACPI functionality in
> > preference to native control.
> >
> > The first two patches in this series are picked from other patchesets aimed at
> > solving similar problems. The last simply unregisters ACPI backlight control
> > on Windows 8 systems when using an Intel GPU. Similar code could be added to
> > other drivers, but I'm reluctant to do so without further investigation as
> > to the behaviour of the vendor drivers under Windows.
>
> Well, after some more time spent on that, we now have a series of 3 patches
> (different from the $subject one) that we think may be used to address this
> issue. As far as I can say, it has been tested by multiple people whose
> systems have those problems and they generally saw improvement.
>
> It is not my ideal approach, but it seems to be the least intrusive and/or
> with the least amount of possible side effects that we can do right now
> as a general measure (alternatively, we could create a possibly long
> blacklist table of affected systems with different workarounds for them,
> but let's just say that is not overwhelmingly attractive).
>
> [1/3] Make ACPICA export things that we need for checking OSI(Win8).
>
> [2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even
> if it is not going to register the backlight interface (needed for
> Thinkpads).
>
> [3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes
> we are Windows 8.
>
> Many thanks to everyone involved!
So this didn't work, as we had to revert [3/3], but I think we should try to
make some progress with this nevertheless. A way forward I'm seeing now could
be to
(1) Split the ACPI video driver so that it is possible to register the
backlight control separately from the event interface.
(2) Add a command line option to i915 to make it use the native backlight
control (without registering the ACPI one) if set. Make unset the
default initially.
(3) Fix i915 backlight control issues for all systems known to have them
(that may take a while) and flip the defailt for that option to set when we
think we're ready.
(4) If there still are problem reports, flip the default back to unset and
repeat (3).
If this converges, everyone will be using the native backlight control by
default and the original problem will go away automatically.
Thoughts?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Wed, 2013-07-31 at 02:01 +0200, Rafael J. Wysocki wrote:
> (3) Fix i915 backlight control issues for all systems known to have them
> (that may take a while) and flip the defailt for that option to set when we
> think we're ready.
Unfortunately I don't have any systems that reproduce problems here, so
I think Intel are going to have to take the lead on this one.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, 2013-07-31 at 02:01 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 18, 2013 02:16:09 AM Rafael J. Wysocki wrote:
> > On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
> > > Windows 8 introduced new policy for backlight control by pushing it out to
> > > graphics drivers. This appears to have coincided with a range of vendors
> > > adding Windows 8 checks to their backlight control code which trigger either
> > > awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
> > > thing to do would be to just disable ACPI backlight control entirely if the
> > > firmware indicates Windows 8 support, but it's entirely possible that
> > > individual graphics drivers might still make use of the ACPI functionality in
> > > preference to native control.
> > >
> > > The first two patches in this series are picked from other patchesets aimed at
> > > solving similar problems. The last simply unregisters ACPI backlight control
> > > on Windows 8 systems when using an Intel GPU. Similar code could be added to
> > > other drivers, but I'm reluctant to do so without further investigation as
> > > to the behaviour of the vendor drivers under Windows.
> >
> > Well, after some more time spent on that, we now have a series of 3 patches
> > (different from the $subject one) that we think may be used to address this
> > issue. As far as I can say, it has been tested by multiple people whose
> > systems have those problems and they generally saw improvement.
> >
> > It is not my ideal approach, but it seems to be the least intrusive and/or
> > with the least amount of possible side effects that we can do right now
> > as a general measure (alternatively, we could create a possibly long
> > blacklist table of affected systems with different workarounds for them,
> > but let's just say that is not overwhelmingly attractive).
> >
> > [1/3] Make ACPICA export things that we need for checking OSI(Win8).
> >
> > [2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even
> > if it is not going to register the backlight interface (needed for
> > Thinkpads).
> >
> > [3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes
> > we are Windows 8.
> >
> > Many thanks to everyone involved!
>
> So this didn't work, as we had to revert [3/3], but I think we should try to
> make some progress with this nevertheless. A way forward I'm seeing now could
> be to
> (1) Split the ACPI video driver so that it is possible to register the
> backlight control separately from the event interface.
> (2) Add a command line option to i915 to make it use the native backlight
> control (without registering the ACPI one) if set. Make unset the
> default initially.
> (3) Fix i915 backlight control issues for all systems known to have them
> (that may take a while) and flip the defailt for that option to set when we
> think we're ready.
> (4) If there still are problem reports, flip the default back to unset and
> repeat (3).
>
> If this converges, everyone will be using the native backlight control by
> default and the original problem will go away automatically.
>
> Thoughts?
>
> Rafael
>
>
Good idea. I have 3-4 laptops with this problem. I can test, but I don't
have devices, on which found regressions (in backlight) in previous
patch.
--
Igor Gnatenko
Fedora release 19 (Schrödinger’s Cat)
Linux 3.10.3-300.fc19.x86_64
On 07/31/2013 08:01 AM, Rafael J. Wysocki wrote:
> On Thursday, July 18, 2013 02:16:09 AM Rafael J. Wysocki wrote:
>> On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
>>> Windows 8 introduced new policy for backlight control by pushing it out to
>>> graphics drivers. This appears to have coincided with a range of vendors
>>> adding Windows 8 checks to their backlight control code which trigger either
>>> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
>>> thing to do would be to just disable ACPI backlight control entirely if the
>>> firmware indicates Windows 8 support, but it's entirely possible that
>>> individual graphics drivers might still make use of the ACPI functionality in
>>> preference to native control.
>>>
>>> The first two patches in this series are picked from other patchesets aimed at
>>> solving similar problems. The last simply unregisters ACPI backlight control
>>> on Windows 8 systems when using an Intel GPU. Similar code could be added to
>>> other drivers, but I'm reluctant to do so without further investigation as
>>> to the behaviour of the vendor drivers under Windows.
>>
>> Well, after some more time spent on that, we now have a series of 3 patches
>> (different from the $subject one) that we think may be used to address this
>> issue. As far as I can say, it has been tested by multiple people whose
>> systems have those problems and they generally saw improvement.
>>
>> It is not my ideal approach, but it seems to be the least intrusive and/or
>> with the least amount of possible side effects that we can do right now
>> as a general measure (alternatively, we could create a possibly long
>> blacklist table of affected systems with different workarounds for them,
>> but let's just say that is not overwhelmingly attractive).
>>
>> [1/3] Make ACPICA export things that we need for checking OSI(Win8).
>>
>> [2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even
>> if it is not going to register the backlight interface (needed for
>> Thinkpads).
>>
>> [3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes
>> we are Windows 8.
>>
>> Many thanks to everyone involved!
>
> So this didn't work, as we had to revert [3/3], but I think we should try to
> make some progress with this nevertheless. A way forward I'm seeing now could
> be to
> (1) Split the ACPI video driver so that it is possible to register the
> backlight control separately from the event interface.
> (2) Add a command line option to i915 to make it use the native backlight
> control (without registering the ACPI one) if set. Make unset the
> default initially.
> (3) Fix i915 backlight control issues for all systems known to have them
> (that may take a while) and flip the defailt for that option to set when we
> think we're ready.
> (4) If there still are problem reports, flip the default back to unset and
> repeat (3).
>
> If this converges, everyone will be using the native backlight control by
> default and the original problem will go away automatically.
>
> Thoughts?
Sounds good to me.
-Aaron
On Sun, Jun 09, 2013 at 07:01:36PM -0400, Matthew Garrett wrote:
> Windows 8 introduced new policy for backlight control by pushing it out to
> graphics drivers. This appears to have coincided with a range of vendors
> adding Windows 8 checks to their backlight control code which trigger either
> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
> thing to do would be to just disable ACPI backlight control entirely if the
> firmware indicates Windows 8 support, but it's entirely possible that
> individual graphics drivers might still make use of the ACPI functionality in
> preference to native control.
Maybe tangential, so Aaron and I were wondering on
https://bugzilla.kernel.org/show_bug.cgi?id=60680 whether it would make
sense to handle the backlight control strictly in the kernel, without
going to userspace and back?
Background is that on my x230, I needed to connect the
Fn-Fx backlight hotkey presses to a script to write to
/sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't
do that (and maybe doesn't have to).
So, without presuming any ACPI or backlight knowledge, can we make the
backlight control work only in the kernel by connecting the hotkey
presses to some backlight controlling interface which backlight-capable
devices implement so that it works regardless of userspace environment?
Even if the machine is not running X?
Hmmm.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 08/07/2013 03:44 PM, Borislav Petkov wrote:
> On Sun, Jun 09, 2013 at 07:01:36PM -0400, Matthew Garrett wrote:
>> Windows 8 introduced new policy for backlight control by pushing it out to
>> graphics drivers. This appears to have coincided with a range of vendors
>> adding Windows 8 checks to their backlight control code which trigger either
>> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest
>> thing to do would be to just disable ACPI backlight control entirely if the
>> firmware indicates Windows 8 support, but it's entirely possible that
>> individual graphics drivers might still make use of the ACPI functionality in
>> preference to native control.
>
> Maybe tangential, so Aaron and I were wondering on
> https://bugzilla.kernel.org/show_bug.cgi?id=60680 whether it would make
> sense to handle the backlight control strictly in the kernel, without
> going to userspace and back?
I think this would require the kernel has the knowledge of which
backlight interface this system is using or should be using, or it
wouldn't know which interface should receive and process the event...
-Aaron
>
> Background is that on my x230, I needed to connect the
> Fn-Fx backlight hotkey presses to a script to write to
> /sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't
> do that (and maybe doesn't have to).
>
> So, without presuming any ACPI or backlight knowledge, can we make the
> backlight control work only in the kernel by connecting the hotkey
> presses to some backlight controlling interface which backlight-capable
> devices implement so that it works regardless of userspace environment?
> Even if the machine is not running X?
>
> Hmmm.
>
> Thanks.
>
On Wed, Aug 07, 2013 at 05:03:15PM +0800, Aaron Lu wrote:
> I think this would require the kernel has the knowledge of which
> backlight interface this system is using or should be using, or it
> wouldn't know which interface should receive and process the event...
Well, we can have a default one, say acpi_video and make it
overridable with command line options like acpi_backlight=vendor or
acpi_backlight=gpu, for example, if acpi_video suffers of a major
suckage...
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, 2013-08-07 at 09:44 +0200, Borislav Petkov wrote:
> Background is that on my x230, I needed to connect the
> Fn-Fx backlight hotkey presses to a script to write to
> /sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't
> do that (and maybe doesn't have to).
>
> So, without presuming any ACPI or backlight knowledge, can we make the
> backlight control work only in the kernel by connecting the hotkey
> presses to some backlight controlling interface which backlight-capable
> devices implement so that it works regardless of userspace environment?
> Even if the machine is not running X?
Not really. The ACPI driver is able to do this because the events and
the backlight are associated with the same device. We could potentially
make something work with GPU backlight drivers since their PCI device
should also be associated with the backlight device, but things get
complicated quickly - once ddcci support is hooked up you'll also have
kernel backlight devices for some external monitors, and now you need to
make a policy decision about which display should be controlled in
response to the keypress. One reasonable choice would be whichever
display contains the currently focused window, which is obviously out of
scope for the kernel.
So if we're going to do this then we need a generalised mechanism
in-kernel for connecting input devices to backlight devices, and we need
a mechanism for disabling it when userspace knows better. This sounds
like a lot of work for something that should really just be handled by
userspace already.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Aug 07, 2013 at 10:36:08AM +0000, Matthew Garrett wrote:
> Not really. The ACPI driver is able to do this because the events and
> the backlight are associated with the same device.
Well, it doesn't work at least in my case.
I need to echo stuff into /sys/class/backlight/intel_backlight :(
> We could potentially make something work with GPU backlight drivers
> since their PCI device should also be associated with the backlight
> device, but things get complicated quickly - once ddcci support
> is hooked up you'll also have kernel backlight devices for some
> external monitors, and now you need to make a policy decision about
> which display should be controlled in response to the keypress. One
> reasonable choice would be whichever display contains the currently
> focused window, which is obviously out of scope for the kernel.
>
> So if we're going to do this then we need a generalised mechanism
> in-kernel for connecting input devices to backlight devices, and we
> need a mechanism for disabling it when userspace knows better. This
> sounds like a lot of work for something that should really just be
> handled by userspace already.
That's easy: single-monitor setups have it enabled by default. So every
laptop out there will just work, without any userspace intervention.
More complicated situations are presented with the opportunity to
disable the kernel-side control and do it themselves.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--