[PATCH] backlight: add support for disabling backlights via sysfs

[PATCH] backlight: add support for disabling backlights via sysfs

[Seth Forshee]

There have been several attempts recently to find ways to disable broken
backlight interfaces on laptops, such as [1]. These haven't always
worked as well as hoped and tend to be driver-specific, so we're seeing
attempts to add an expanding number of interfaces between drivers to try
to cope with the situation, such as [2] and [3]. We're also seeing
difficulties properly expressing the inter-module dependencies[4].

Rather than trying to address this situation in a piecemeal fashion, we
should find a solution that deal with disabling broken backlights more
generically. This patch does so by adding an "enabled" attribute to
sysfs for backlight devices. Writing 0 to this attribute disables the
backlight, blocking most attempts to change the state. Tools like udev
can set use this attribute to disable known broken backlight interfaces,
and tools like gnome-settings-daemon can query the attribute to avoid
using disabled backlights.

[1] https://lkml.org/lkml/2012/3/22/156
[2] https://lkml.org/lkml/2012/3/15/529
[3] https://lkml.org/lkml/2012/6/13/48
[4] https://lkml.org/lkml/2012/6/28/159

Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Corentin Chary <corentin.chary@gmail.com>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---

I wanted to send this out for comments to see how people felt about this
idea. If it's well received I can follow up with patches to remove the
acpi_video_unregister() and apple_bl_unregister() calls currently being
used for this purpose.

 Documentation/ABI/stable/sysfs-class-backlight |   10 +++++
 drivers/video/backlight/backlight.c            |   54 +++++++++++++++++++++---
 include/linux/backlight.h                      |    1 +
 3 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-class-backlight b/Documentation/ABI/stable/sysfs-class-backlight
index 70302f3..3b3c477 100644
--- a/Documentation/ABI/stable/sysfs-class-backlight
+++ b/Documentation/ABI/stable/sysfs-class-backlight
@@ -54,3 +54,13 @@ Description:
 		backlight state. Platform interfaces are mostly a
 		holdover from pre-standardisation of firmware
 		interfaces.
+
+What:		/sys/class/backlight/<backlight>/enabled
+Date:		May 2012
+KernelVersion:	3.6
+Contact:	Seth Forshee <seth.forshee@canonical.com>
+Description:
+		Enable or disable this <backlight>. Also shows whether
+		the backlight is enabled. This can be used by tools like
+		udev to disable backlights that are known to be broken
+		on a given platform.
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 297db2f..5275738 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -53,7 +53,8 @@ static int fb_notifier_callback(struct notifier_block *self,
 				bd->props.state &= ~BL_CORE_FBBLANK;
 			else
 				bd->props.state |= BL_CORE_FBBLANK;
-			backlight_update_status(bd);
+			if (!(bd->props.state & BL_CORE_DISABLED))
+				backlight_update_status(bd);
 		}
 	mutex_unlock(&bd->ops_lock);
 	return 0;
@@ -124,7 +125,7 @@ static ssize_t backlight_store_power(struct device *dev,
 
 	rc = -ENXIO;
 	mutex_lock(&bd->ops_lock);
-	if (bd->ops) {
+	if (!(bd->props.state & BL_CORE_DISABLED) && bd->ops) {
 		pr_debug("set power to %lu\n", power);
 		if (bd->props.power != power) {
 			bd->props.power = power;
@@ -159,7 +160,7 @@ static ssize_t backlight_store_brightness(struct device *dev,
 	rc = -ENXIO;
 
 	mutex_lock(&bd->ops_lock);
-	if (bd->ops) {
+	if (!(bd->props.state & BL_CORE_DISABLED) && bd->ops) {
 		if (brightness > bd->props.max_brightness)
 			rc = -EINVAL;
 		else {
@@ -206,6 +207,43 @@ static ssize_t backlight_show_actual_brightness(struct device *dev,
 	return rc;
 }
 
+static ssize_t backlight_show_enabled(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct backlight_device *bd = to_backlight_device(dev);
+
+	return sprintf(buf, "%d\n", !(bd->props.state & BL_CORE_DISABLED));
+}
+
+static ssize_t backlight_store_enabled(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct backlight_device *bd = to_backlight_device(dev);
+	int rc;
+	unsigned long enabled;
+
+	rc = kstrtoul(buf, 0, &enabled);
+	if (rc)
+		return rc;
+	if (enabled != 0 && enabled != 1)
+		return -EINVAL;
+
+	mutex_lock(&bd->ops_lock);
+
+	if (enabled)
+		bd->props.state &= ~BL_CORE_DISABLED;
+	else
+		bd->props.state |= BL_CORE_DISABLED;
+
+	if (bd->ops)
+		backlight_update_status(bd);
+
+	mutex_unlock(&bd->ops_lock);
+
+	sysfs_notify(&bd->dev.kobj, NULL, "enabled");
+	return count;
+}
+
 static struct class *backlight_class;
 
 static int backlight_suspend(struct device *dev, pm_message_t state)
@@ -213,7 +251,8 @@ static int backlight_suspend(struct device *dev, pm_message_t state)
 	struct backlight_device *bd = to_backlight_device(dev);
 
 	mutex_lock(&bd->ops_lock);
-	if (bd->ops && bd->ops->options & BL_CORE_SUSPENDRESUME) {
+	if (!(bd->props.state & BL_CORE_DISABLED) &&
+	    bd->ops && bd->ops->options & BL_CORE_SUSPENDRESUME) {
 		bd->props.state |= BL_CORE_SUSPENDED;
 		backlight_update_status(bd);
 	}
@@ -227,7 +266,8 @@ static int backlight_resume(struct device *dev)
 	struct backlight_device *bd = to_backlight_device(dev);
 
 	mutex_lock(&bd->ops_lock);
-	if (bd->ops && bd->ops->options & BL_CORE_SUSPENDRESUME) {
+	if (!(bd->props.state & BL_CORE_DISABLED) &&
+	    bd->ops && bd->ops->options & BL_CORE_SUSPENDRESUME) {
 		bd->props.state &= ~BL_CORE_SUSPENDED;
 		backlight_update_status(bd);
 	}
@@ -250,6 +290,7 @@ static struct device_attribute bl_device_attributes[] = {
 		     NULL),
 	__ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
 	__ATTR(type, 0444, backlight_show_type, NULL),
+	__ATTR(enabled, 0644, backlight_show_enabled, backlight_store_enabled),
 	__ATTR_NULL,
 };
 
@@ -265,7 +306,8 @@ void backlight_force_update(struct backlight_device *bd,
 			    enum backlight_update_reason reason)
 {
 	mutex_lock(&bd->ops_lock);
-	if (bd->ops && bd->ops->get_brightness)
+	if (!(bd->props.state & BL_CORE_DISABLED) &&
+	    bd->ops && bd->ops->get_brightness)
 		bd->props.brightness = bd->ops->get_brightness(bd);
 	mutex_unlock(&bd->ops_lock);
 	backlight_generate_event(bd, reason);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5ffc6dd..31917d0 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -77,6 +77,7 @@ struct backlight_properties {
 
 #define BL_CORE_SUSPENDED	(1 << 0)	/* backlight is suspended */
 #define BL_CORE_FBBLANK		(1 << 1)	/* backlight is under an fb blank event */
+#define BL_CORE_DISABLED	(1 << 2)	/* backlight disabled by userspace */
 #define BL_CORE_DRIVER4		(1 << 28)	/* reserved for driver specific use */
 #define BL_CORE_DRIVER3		(1 << 29)	/* reserved for driver specific use */
 #define BL_CORE_DRIVER2		(1 << 30)	/* reserved for driver specific use */
-- 
1.7.9.5

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Matthew Garrett]

On Thu, Jun 28, 2012 at 02:07:06PM -0500, Seth Forshee wrote:

> Rather than trying to address this situation in a piecemeal fashion, we
> should find a solution that deal with disabling broken backlights more
> generically. This patch does so by adding an "enabled" attribute to
> sysfs for backlight devices. Writing 0 to this attribute disables the
> backlight, blocking most attempts to change the state. Tools like udev
> can set use this attribute to disable known broken backlight interfaces,
> and tools like gnome-settings-daemon can query the attribute to avoid
> using disabled backlights.

I'm not entirely thrilled by this, especially because in several cases I 
suspect that we're just going to end up disabling acpi_backlight rather 
than fixing any of the range of integration bugs we still have with it. 
If anyone has links with OEMs then I'd love to know how Windows handles 
backlight control policy, but otherwise I think Corentin's approach of 
having the vendor drivers promote or demote themselves makes more sense 
than pushing the problem out to userspace.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Seth Forshee]

On Thu, Jun 28, 2012 at 08:10:43PM +0100, Matthew Garrett wrote:
> On Thu, Jun 28, 2012 at 02:07:06PM -0500, Seth Forshee wrote:
> 
> > Rather than trying to address this situation in a piecemeal fashion, we
> > should find a solution that deal with disabling broken backlights more
> > generically. This patch does so by adding an "enabled" attribute to
> > sysfs for backlight devices. Writing 0 to this attribute disables the
> > backlight, blocking most attempts to change the state. Tools like udev
> > can set use this attribute to disable known broken backlight interfaces,
> > and tools like gnome-settings-daemon can query the attribute to avoid
> > using disabled backlights.
> 
> I'm not entirely thrilled by this, especially because in several cases I 
> suspect that we're just going to end up disabling acpi_backlight rather 
> than fixing any of the range of integration bugs we still have with it. 
> If anyone has links with OEMs then I'd love to know how Windows handles 
> backlight control policy, but otherwise I think Corentin's approach of 
> having the vendor drivers promote or demote themselves makes more sense 
> than pushing the problem out to userspace.

I actually don't think Corentin's solution is a bad one. It does suffer
from a couple of shortcomings though. First, it only works for broken
ACPI backlights, and some platforms have other backlight interfaces that
are broken (e.g. the i915 backlight on the MacBook Pro 8,2). Second,
marking backlights as broken in the kernel necessitates ever-expanding
dmi blacklists in some of the platform drivers, unless we can get
vendors to stop providing broken backlight interfaces.

I'm all for fixing integration bugs in the ACPI backlight
implementations if we can, but some vendor implementations are just
flat-out broken, and it isn't always possible to get vendor cooperation.
In the case of Toshiba I've tried reaching out to them to work on ACPI
integration issues, but they flat out refused.

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Matthew Garrett]

On Thu, Jun 28, 2012 at 02:30:17PM -0500, Seth Forshee wrote:

> I actually don't think Corentin's solution is a bad one. It does suffer
> from a couple of shortcomings though. First, it only works for broken
> ACPI backlights, and some platforms have other backlight interfaces that
> are broken (e.g. the i915 backlight on the MacBook Pro 8,2). Second,
> marking backlights as broken in the kernel necessitates ever-expanding
> dmi blacklists in some of the platform drivers, unless we can get
> vendors to stop providing broken backlight interfaces.

Userspace should already be prioritising platform interfaces over raw 
interfaces, so if gmux works on the Mac then there's no problem. DMI 
lists should, broadly speaking, be unnecessary - they're mostly a 
symptom of us not understanding how the hardware is expected to work.

> I'm all for fixing integration bugs in the ACPI backlight
> implementations if we can, but some vendor implementations are just
> flat-out broken, and it isn't always possible to get vendor cooperation.
> In the case of Toshiba I've tried reaching out to them to work on ACPI
> integration issues, but they flat out refused.

We already know that our implementation of the IGD opregion is broken, 
but Intel won't hand over newer versions of the spec. If you see 
problems with the acpi backlight interface on Intel graphics then that 
should be the default assumption.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Seth Forshee]

On Thu, Jun 28, 2012 at 08:36:52PM +0100, Matthew Garrett wrote:
> Userspace should already be prioritising platform interfaces over raw 
> interfaces, so if gmux works on the Mac then there's no problem. DMI 
> lists should, broadly speaking, be unnecessary - they're mostly a 
> symptom of us not understanding how the hardware is expected to work.

I agree that we want to avoid papering over problems instead of fixing
them, but we're already disabling the ACPI backlight in some cases and
will continue to do so for the time being. We're also doing it with
apple_bl. It's not a question of whether we do it, but how.

What I still don't understand is your reason for preferring
driver-specific interfaces for disabling backlights to a single generic
interface. Are you afraid that allowing it to be done from userspace
will make it too easy, and as a result quirks will be applied without
attempting to fix the problem (i.e. the quriking should go through a
kernel dev to ensure someone tries to fix the problem is made before
applying a quirk)?

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Matthew Garrett]

On Thu, Jun 28, 2012 at 04:16:00PM -0500, Seth Forshee wrote:

> What I still don't understand is your reason for preferring
> driver-specific interfaces for disabling backlights to a single generic
> interface. Are you afraid that allowing it to be done from userspace
> will make it too easy, and as a result quirks will be applied without
> attempting to fix the problem (i.e. the quriking should go through a
> kernel dev to ensure someone tries to fix the problem is made before
> applying a quirk)?

Yup, that. We'll just end up with three billion forum posts telling 
people to add a line to a text file and nobody will ever fix the real 
problem.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[joeyli]

於 四，2012-06-28 於 22:19 +0100，Matthew Garrett 提到：
> On Thu, Jun 28, 2012 at 04:16:00PM -0500, Seth Forshee wrote:
> 
> > What I still don't understand is your reason for preferring
> > driver-specific interfaces for disabling backlights to a single generic
> > interface. Are you afraid that allowing it to be done from userspace
> > will make it too easy, and as a result quirks will be applied without
> > attempting to fix the problem (i.e. the quriking should go through a
> > kernel dev to ensure someone tries to fix the problem is made before
> > applying a quirk)?
> 
> Yup, that. We'll just end up with three billion forum posts telling 
> people to add a line to a text file and nobody will ever fix the real 
> problem.
> 

Yes, we can not just hide this issue. It's entirely a manufacturers'
problem.

More and more machines have broken _BCM is because ODM didn't really
follow WDDM spec to maintain standard acpi interface for compatibility,
e.g. XP or Linux that don't support WDDM:
	http://msdn.microsoft.com/en-us/windows/hardware/gg487382.aspx

In WDDM architecture, in spec p.5, there have a "Monitor" driver to
check the WDDM driver support brightness DDI, if not, then it will use
standard acpi method to control brightness.

Unfortunately, no OEM preload Windows XP, now. So, more and more ODM
didn't test _BCM, actaully, they never test it, ODM QA just test the
brightness control UI available on newest Windows version (Windows 7).

We cann't just follow windows approach because it causes standard acpi
method neglected. If we also do that, then _BCM function will have no
chance to fix by any manufacturers because it's really nobody care.

Currently, on Linux we default use _BCM to control brightness, video
driver is a fallback. And we enable this fallback in platform driver.
It can keep still have people pay attention on standard acpi methods.


Thanks a lot!
Joey Lee

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Matthew Garrett]

On Fri, Jun 29, 2012 at 10:43:59AM +0800, joeyli wrote:

> 	http://msdn.microsoft.com/en-us/windows/hardware/gg487382.aspx
>
> In WDDM architecture, in spec p.5, there have a "Monitor" driver to
> check the WDDM driver support brightness DDI, if not, then it will use
> standard acpi method to control brightness.

What's the interaction between GPU drivers and platform drivers? For 
instance, on acer systems that have backlight control via WMI, does 
Windows use WMI, the GPU driver or ACPI?
 
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[joeyli]

於 五，2012-06-29 於 03:57 +0100，Matthew Garrett 提到：
> On Fri, Jun 29, 2012 at 10:43:59AM +0800, joeyli wrote:
> 
> > 	http://msdn.microsoft.com/en-us/windows/hardware/gg487382.aspx
> >
> > In WDDM architecture, in spec p.5, there have a "Monitor" driver to
> > check the WDDM driver support brightness DDI, if not, then it will use
> > standard acpi method to control brightness.
> 
> What's the interaction between GPU drivers and platform drivers? For 
> instance, on acer systems that have backlight control via WMI, does 
> Windows use WMI, the GPU driver or ACPI?
>  

I think GPU driver.

Because there have no wmi method support backlight control on modern
Acer machine and _BCM also broken on some machines.


On old machines that have 6AF4F258 wmi method, it supported to set
brightness level through wmi. But 6AF4F258 is really old now and almost
broken or not provide by new acer BIOS.

So, it is clear for modern Acer machines do NOT use wmi to control
backlight. It must control brightness through acpi or GPU driver.
(For very very new machine maybe changed again, need to check.)

Have a a period of time, acer BIOS provide healthful _BCM
implementation, in _BCM, they changed brightness through OPregion or
write EC register.

But now, some machines have broken _BCM and they are also not control
brightness through WMI, that means the brightness control is through GPU
driver.

Per WDDM spec, if WDDM driver support DDI, the NORMAL code path of
brightness control is:
	Mobility Center(userland) -> Monitor Driver -> WDDM Driver -> Graphics Adapter


Reviewed DSDT from Acer machines, I believe Acer didn't force ODM fully implement
Acer's WMI space. So, also need to check with Acer for how many ODM really follow WDDM spec.






 

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Corentin Chary]

On Thu, Jun 28, 2012 at 9:36 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Thu, Jun 28, 2012 at 02:30:17PM -0500, Seth Forshee wrote:
>
>> I actually don't think Corentin's solution is a bad one. It does suffer
>> from a couple of shortcomings though. First, it only works for broken
>> ACPI backlights, and some platforms have other backlight interfaces that
>> are broken (e.g. the i915 backlight on the MacBook Pro 8,2). Second,
>> marking backlights as broken in the kernel necessitates ever-expanding
>> dmi blacklists in some of the platform drivers, unless we can get
>> vendors to stop providing broken backlight interfaces.
>
> Userspace should already be prioritising platform interfaces over raw
> interfaces, so if gmux works on the Mac then there's no problem.

Hehe, sometime the platform interface doesn't work and the raw does.
That's the case on various samsung-laptop since we have absolutely no
documentation about SABI and no known way to probe if the
implementation is working or not. But anyway, for samsung-laptop it's
disabled by default in favor of acpi_video (which is also broken most
of the time on samsung laptops).

> DMI
> lists should, broadly speaking, be unnecessary - they're mostly a
> symptom of us not understanding how the hardware is expected to work.
>
>> I'm all for fixing integration bugs in the ACPI backlight
>> implementations if we can, but some vendor implementations are just
>> flat-out broken, and it isn't always possible to get vendor cooperation.
>> In the case of Toshiba I've tried reaching out to them to work on ACPI
>> integration issues, but they flat out refused.
>
> We already know that our implementation of the IGD opregion is broken,
> but Intel won't hand over newer versions of the spec. If you see
> problems with the acpi backlight interface on Intel graphics then that
> should be the default assumption.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org



-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH] backlight: add support for disabling backlights via sysfs

[Seth Forshee]

On Fri, Jun 29, 2012 at 02:24:38PM +0200, Corentin Chary wrote:
> On Thu, Jun 28, 2012 at 9:36 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Thu, Jun 28, 2012 at 02:30:17PM -0500, Seth Forshee wrote:
> >
> >> I actually don't think Corentin's solution is a bad one. It does suffer
> >> from a couple of shortcomings though. First, it only works for broken
> >> ACPI backlights, and some platforms have other backlight interfaces that
> >> are broken (e.g. the i915 backlight on the MacBook Pro 8,2). Second,
> >> marking backlights as broken in the kernel necessitates ever-expanding
> >> dmi blacklists in some of the platform drivers, unless we can get
> >> vendors to stop providing broken backlight interfaces.
> >
> > Userspace should already be prioritising platform interfaces over raw
> > interfaces, so if gmux works on the Mac then there's no problem.
> 
> Hehe, sometime the platform interface doesn't work and the raw does.
> That's the case on various samsung-laptop since we have absolutely no
> documentation about SABI and no known way to probe if the
> implementation is working or not. But anyway, for samsung-laptop it's
> disabled by default in favor of acpi_video (which is also broken most
> of the time on samsung laptops).

The situation is similar with Toshibas. On some the raw interface is
still the only reliable way to change the backlight. I just got my hands
on one of these models though, so I'm hoping to make some progress on
that front in the near future.