[RFC PATCH 0/2] i915 brightness control

[RFC PATCH 0/2] i915 brightness control

[Kamal Mostafa]

Requesting comments on this idea (proposed by Matthew Garrett) and my
implementation.  Note especially the KNOWN PROBLEM with a "GM45" GPU.

 -Kamal

.......

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/568611

In order to avoid the often-dysfunctional native acpi brightness control
methods, a new acpi_brightness_hook interface is made available.

The i915 driver uses the new hook to take over brightness control.
Boot with i915.brightness=0 to disable.

KNOWN PROBLEM:
  - i915 opregion brightness control silently fails on GM45 (pci-id 0x2a42)

[RFC PATCH 1/2] acpi/video: acpi_brightness_hook API
[RFC PATCH 2/2] drm/i915: override acpi brightness control

[RFC PATCH 1/2] acpi/video: acpi_brightness_hook API

[Kamal Mostafa]

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/568611

New acpi_brightness_hook_register lets another driver (e.g. i915) override
the often-dysfunctional native acpi brightness control methods.

Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 drivers/acpi/video.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/acpi/video.h |   10 +++
 2 files changed, 147 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index bd9843a..331fdcc 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -481,6 +481,12 @@ acpi_video_device_set_state(struct acpi_video_device *device, int state)
 	return status;
 }
 
+static unsigned int (*acpi_brightness_hook_routine)
+			(void *dev, unsigned int brightness) = NULL;
+static char *acpi_brightness_hook_driver;
+static void *acpi_brightness_hook_dev;
+static unsigned int acpi_brightness_hook_max;
+
 static int
 acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
 				   union acpi_object **levels)
@@ -520,13 +526,25 @@ acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
 	struct acpi_object_list args = { 1, &arg0 };
 	int state;
 
-	arg0.integer.value = level;
+	/* If another driver has registered a brightness hook override,
+	 * set the brightness using that method, otherwise set the brightness
+	 * using the acpi _BCM method. */
+	if ( acpi_brightness_hook_routine ) {
+		status = acpi_brightness_hook_routine(acpi_brightness_hook_dev,
+				level * acpi_brightness_hook_max / 100);
+		if ( status != 0 ) {
+			ACPI_ERROR((AE_INFO, "brightness hook failed"));
+			return -EIO;
+		}
+	} else {
+		arg0.integer.value = level;
 
-	status = acpi_evaluate_object(device->dev->handle, "_BCM",
-				      &args, NULL);
-	if (ACPI_FAILURE(status)) {
-		ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
-		return -EIO;
+		status = acpi_evaluate_object(device->dev->handle, "_BCM",
+					      &args, NULL);
+		if (ACPI_FAILURE(status)) {
+			ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
+			return -EIO;
+		}
 	}
 
 	device->brightness->curr = level;
@@ -607,7 +625,10 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 	acpi_status status = AE_OK;
 	int i;
 
-	if (device->cap._BQC || device->cap._BCQ) {
+	/* Try to get the current brightness using the _BQC/_BCQ method, only
+	 * if another driver has not registered a brightness hook override. */
+	if (!acpi_brightness_hook_routine
+			&& (device->cap._BQC || device->cap._BCQ)) {
 		char *buf = device->cap._BQC ? "_BQC" : "_BCQ";
 
 		status = acpi_evaluate_integer(device->dev->handle, buf,
@@ -784,7 +805,7 @@ acpi_video_cmp_level(const void *a, const void *b)
  *  	device	: video output device (LCD, CRT, ..)
  *
  *  Return Value:
- *	Maximum brightness level
+ *  	0 on success, error code on failure.
  *
  *  Allocate and initialize device->brightness.
  */
@@ -944,6 +965,99 @@ out:
  *	device	: video output device (LCD, CRT, ..)
  *
  *  Return Value:
+ *  	0 on success, error code on failure.
+ *
+ *  Allocate and initialize device->brightness. when a driver has registered
+ *  a brightness hook override via acpi_brightness_hook_register.
+ *
+ *  Cobbles up a fake brightness 'levels' array (emulating a _BCL list) and
+ *  sets max brightness -- effectively what acpi_video_init_brightness does
+ *  for the native acpi brightness methods
+ */
+static int
+acpi_brightness_hook_init(struct acpi_video_device *device)
+{
+	struct acpi_video_device_brightness *br = NULL;
+	int result = -EINVAL;
+	int nsteps = 10;
+	int count, i;
+	static int initialized = 0;
+
+	if ( initialized )
+	    return 1;
+	initialized = 1;
+
+	device->brightness = NULL;
+
+	br = kzalloc(sizeof(*br), GFP_KERNEL);
+	if (!br) {
+		printk(KERN_ERR "can't allocate memory\n");
+		result = -ENOMEM;
+		return result;
+	}
+	br->levels = kmalloc((nsteps + 2) * sizeof *(br->levels),
+				GFP_KERNEL);
+	if (!br->levels) {
+		result = -ENOMEM;
+		kfree(br);
+		return result;
+	}
+
+	for (count=2, i = 1; i <= nsteps; i++)
+		br->levels[count++] = (u32) i * 100 / nsteps;
+	br->levels[0] = 100;
+	br->levels[1] = br->levels[2+(nsteps/2)];
+	br->count = count;
+	device->brightness = br;
+
+	result = acpi_video_device_lcd_set_level(device, 100);
+	if (result) {
+	    kfree(br->levels);
+	    kfree(br);
+	    device->brightness = NULL;
+	    return result;
+	}
+
+	/* Switch off acpi's native brightness switch control */
+	brightness_switch_enabled = 0;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			  "set up %d brightness levels\n", nsteps));
+
+	return 0;
+}
+
+/*
+ *  Arg:
+ *	driver_name	: name of the registering driver
+ *	set_brightness_routine:	pointer to the new set_brightness method
+ *	dev		: arbitrary pointer passed to set_brightness_routine
+ *	max_brightness	: set_brightnes_routines' maximum brightness value
+ *
+ *  Return Value:
+ *  	None
+ *
+ *  Register a brightness hook override method, which ACPI will use
+ *  instead of its native _BCM/_BCL/_BQC methods.
+ */
+void acpi_brightness_hook_register(
+		char *driver_name,
+		unsigned int (*set_brightness_routine)
+				(void *dev, unsigned int brightness),
+		void *dev, int max_brightness)
+{
+	acpi_brightness_hook_routine = set_brightness_routine;
+	acpi_brightness_hook_driver = driver_name,
+	acpi_brightness_hook_dev = dev;
+	acpi_brightness_hook_max = max_brightness;
+}
+EXPORT_SYMBOL(acpi_brightness_hook_register);
+
+/*
+ *  Arg:
+ *	device	: video output device (LCD, CRT, ..)
+ *
+ *  Return Value:
  *  	None
  *
  *  Find out all required AML methods defined under the output
@@ -984,22 +1098,33 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 		device->cap._DSS = 1;
 	}
 
-	if (acpi_video_backlight_support()) {
+	if (acpi_brightness_hook_routine || acpi_video_backlight_support()) {
 		int result;
 		static int count = 0;
 		char *name;
 
-		result = acpi_video_init_brightness(device);
+		/* If another driver has registered a brightness hook override,
+		 * call the brightness_hook init, otherwise call the native
+		 * acpi_video brightness init. */
+		if (acpi_brightness_hook_routine)
+			result = acpi_brightness_hook_init(device);
+		else
+			result = acpi_video_init_brightness(device);
 		if (result)
 			return;
 		name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
 		if (!name)
 			return;
 
-		sprintf(name, "acpi_video%d", count++);
+		if (acpi_brightness_hook_routine)
+			sprintf(name, "%s", acpi_brightness_hook_driver);
+		else
+			sprintf(name, "acpi_video%d", count++);
 		device->backlight = backlight_device_register(name,
 			NULL, device, &acpi_backlight_ops);
 		device->backlight->props.max_brightness = device->brightness->count-3;
+		dev_info(&device->dev->dev,
+				"registered as backlight/%s\n", name);
 		kfree(name);
 
 		result = sysfs_create_link(&device->backlight->dev.kobj,
diff --git a/include/acpi/video.h b/include/acpi/video.h
index cf7be3d..645b574 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -2,9 +2,19 @@
 #define __ACPI_VIDEO_H
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
+extern void acpi_brightness_hook_register(
+		char *driver_name,
+		unsigned int (*set_brightness_routine)
+				(void *dev, unsigned int brightness),
+		void *dev, unsigned int max_brightness);
 extern int acpi_video_register(void);
 extern void acpi_video_unregister(void);
 #else
+static inline acpi_brightness_hook_register(
+		char *driver_name,
+		unsigned int (*set_brightness_routine)
+				(void *dev, unsigned int brightness),
+		void *dev, unsigned int max_brightness) { return; }
 static inline int acpi_video_register(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 #endif
-- 
1.7.0.4

[RFC PATCH 2/2] drm/i915: override acpi brightness control

[Kamal Mostafa]

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/568611

Override acpi brightness control with i915-opregion method.

Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 drivers/gpu/drm/i915/i915_opregion.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_opregion.c b/drivers/gpu/drm/i915/i915_opregion.c
index 7cc8410..a69033e 100644
--- a/drivers/gpu/drm/i915/i915_opregion.c
+++ b/drivers/gpu/drm/i915/i915_opregion.c
@@ -32,6 +32,9 @@
 #include "i915_drm.h"
 #include "i915_drv.h"
 
+unsigned int i915_brightness = 1;
+module_param_named(brightness, i915_brightness, int, 0400);
+
 #define PCI_ASLE 0xe4
 #define PCI_LBPC 0xf4
 #define PCI_ASLS 0xfc
@@ -422,6 +425,18 @@ static void intel_didl_outputs(struct drm_device *dev)
 		opregion->acpi->didl[i] = 0;
 }
 
+static unsigned int i915_set_brightness(void *dev, unsigned int brightness)
+{
+        /* Nb. brightness value range is 0 to 255. */
+    	u32 bclp = ASLE_BCLP_VALID | brightness;
+	struct drm_device *drmdev = dev;
+
+	if (IS_IRONLAKE(drmdev))
+	    return asle_set_backlight_ironlake(drmdev, bclp);
+	else
+	    return asle_set_backlight(drmdev, bclp);
+}
+
 int intel_opregion_init(struct drm_device *dev, int resume)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -469,6 +484,10 @@ int intel_opregion_init(struct drm_device *dev, int resume)
 		DRM_DEBUG_DRIVER("ASLE supported\n");
 		opregion->asle = base + OPREGION_ASLE_OFFSET;
 		opregion_enable_asle(dev);
+	        /* Register the i915-based brightness control with ACPI */
+		if (!resume && i915_brightness)
+			acpi_brightness_hook_register("i915",
+					i915_set_brightness, dev, 255);
 	}
 
 	if (!resume)
-- 
1.7.0.4

Re: [Intel-gfx] [RFC PATCH 0/2] i915 brightness control

[Pedro Ribeiro]

On 2 June 2010 23:11, Kamal Mostafa <kamal@canonical.com> wrote:
> Requesting comments on this idea (proposed by Matthew Garrett) and my
> implementation.  Note especially the KNOWN PROBLEM with a "GM45" GPU.
>
>  -Kamal
>
> .......
>
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/568611
>
> In order to avoid the often-dysfunctional native acpi brightness control
> methods, a new acpi_brightness_hook interface is made available.
>
> The i915 driver uses the new hook to take over brightness control.
> Boot with i915.brightness=0 to disable.
>
> KNOWN PROBLEM:
>  - i915 opregion brightness control silently fails on GM45 (pci-id 0x2a42)
>
> [RFC PATCH 1/2] acpi/video: acpi_brightness_hook API
> [RFC PATCH 2/2] drm/i915: override acpi brightness control
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Are you sure this is a good idea to enable it by default just because
of a problem with one manufacturer (Dell)?

My Lenovo laptop has fine brightness control with a GM45 and this may break it.
Might be safer only to enable it for laptops with known problems.


Regards,
Pedro
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Intel-gfx] [RFC PATCH 0/2] i915 brightness control

[Matthew Garrett]

On Thu, Jun 03, 2010 at 12:20:39AM +0100, Pedro Ribeiro wrote:

> Are you sure this is a good idea to enable it by default just because
> of a problem with one manufacturer (Dell)?

It should (a-ha ha) be entirely safe on systems using the IGD opregion 
spec.

> My Lenovo laptop has fine brightness control with a GM45 and this may break it.
> Might be safer only to enable it for laptops with known problems.

You have a complete list of all laptops with problems in this area? The 
ongoing situation with Windows 7 compatibility breaking machines that 
previously worked suggests that there's something going on which we 
don't understand or implement.

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

Re: [Intel-gfx] [RFC PATCH 0/2] i915 brightness control

[Jerone Young]

On Thu, 2010-06-03 at 00:20 +0100, Pedro Ribeiro wrote:
> On 2 June 2010 23:11, Kamal Mostafa <kamal@canonical.com> wrote:
> > Requesting comments on this idea (proposed by Matthew Garrett) and my
> > implementation.  Note especially the KNOWN PROBLEM with a "GM45" GPU.
> >
> >  -Kamal
> >
> > .......
> >
> > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/568611
> >
> > In order to avoid the often-dysfunctional native acpi brightness control
> > methods, a new acpi_brightness_hook interface is made available.
> >
> > The i915 driver uses the new hook to take over brightness control.
> > Boot with i915.brightness=0 to disable.
> >
> > KNOWN PROBLEM:
> >  - i915 opregion brightness control silently fails on GM45 (pci-id 0x2a42)
> >
> > [RFC PATCH 1/2] acpi/video: acpi_brightness_hook API
> > [RFC PATCH 2/2] drm/i915: override acpi brightness control
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> Are you sure this is a good idea to enable it by default just because
> of a problem with one manufacturer (Dell)?

This issue is with many manufactures. From this bug you can see HP
machines are effected also.

https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/568611
> 
> My Lenovo laptop has fine brightness control with a GM45 and this may break it.
> Might be safer only to enable it for laptops with known problems.
> 
> 
> Regards,
> Pedro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/2] i915 brightness control

[Matthew Garrett]

On Wed, Jun 02, 2010 at 03:11:40PM -0700, Kamal Mostafa wrote:

> In order to avoid the often-dysfunctional native acpi brightness control
> methods, a new acpi_brightness_hook interface is made available.
> 
> The i915 driver uses the new hook to take over brightness control.
> Boot with i915.brightness=0 to disable.

I've looked into this issue more closely and think I've worked out the 
underlying problem. The system in question appears to have two GPUs and 
exposes two ACPI backlight devices. Both of these are associated with 
existing PCI devices, so we don't ignore either of them because of that. 
Further, one of them (the AMD one) implements the spec properly and 
should work. We don't seem to perform a more fine-grained check to 
identify whether every ACPI backlight has all the required methods, and 
so as a result we provide both the working one and the non-working one.

Having thought about this some more, I don't think this is the right 
approach. We should be ensuring that every backlight ahs all the 
required methods and then dropping the one that doesn't. This should be 
replaced with a native i915 backlight, and I sent patches to do that 
last week.

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

Re: [RFC PATCH 0/2] i915 brightness control

[Kamal Mostafa]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

On Mon, 2010-09-13 at 18:48 +0100, Matthew Garrett wrote:
> I've looked into this issue more closely and think I've worked out the 
> underlying problem. The system in question appears to have two GPUs and 
> exposes two ACPI backlight devices. Both of these are associated with 
> existing PCI devices, so we don't ignore either of them because of that. 
> Further, one of them (the AMD one) implements the spec properly and 
> should work. We don't seem to perform a more fine-grained check to 
> identify whether every ACPI backlight has all the required methods, and 
> so as a result we provide both the working one and the non-working one.
> 
> Having thought about this some more, I don't think this is the right 
> approach. We should be ensuring that every backlight ahs all the 
> required methods and then dropping the one that doesn't. This should be 
> replaced with a native i915 backlight, and I sent patches to do that 
> last week.


I agree.  Your proposed design is good, and I have successfully tested
your proposed patches[1] (after minor porting changes to Ubuntu
Maverick's 2.6.35).  Thanks very much Matthew!

FYI, I have published an experimental Ubuntu Maverick PPA kernel[2]
which includes your patches, plus my dell_laptop tweaks to inhibit the
broken dell_backlight by a module param or dmi blacklist table (in lieu
of a yet to be implemented more fine-grained check).

 -Kamal Mostafa <kamal@canonical.com>


[1] 2010-09-08 [Intel-gfx] [PATCH] i915: Add native backlight control
    2010-09-08 [Intel-gfx] [PATCH] Backlight: Add backlight type

[2] https://launchpad.net/~kamalmostafa/+archive/linux-kamal-mjgbacklight


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]