[PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Thomas Renninger]

Can these two go to linux-next or -mm, pls.
If they got some testing, maybe it's even possible to still queue them
for 2.6.27?

Be aware that they depend on a patch that got queued up for 2.6.27 in
the ACPI tree:
Subject: fix brightness allocation
By: Julia Jomantaite <julia.jomantaite@gmail.com>

Both are compile tested.


Before I adjusted them a bit to Julia's patch and reordered the functions a 
bit I tested them on a Lenovo T61 with an internal Intel Graphics card with 
IGD BIOS support and a T61 with a Nvidida card.
In both cases the correct ACPI graphics device is used and brightness worked 
out of the box.

---
ACPI: video: Ignore devices that aren't present in hardware

This is a reimplemention of commit
0119509c4fbc9adcef1472817fda295334612976
from Matthew Garrett <mjg59@srcf.ucam.org>

This patch got removed because of a regression: ThinkPads with a
Intel graphics card and an Integrated Graphics Device BIOS implementation
stopped working.
In fact, they only worked because the wrong ACPI device  
was used (via int10). So ACPI functions were poking on the wrong
hardware which is a sever bug.
The next patch provides support for above ThinkPads to be able to
switch brightness via the legacy thinkpad_acpi driver and automatically
detect when to use it.

Original commit message from Matthew Garrett:
    Vendors often ship machines with a choice of integrated or discrete
    graphics, and use the same DSDT for both. As a result, the ACPI video
    module will locate devices that may not exist on this specific platform.
    Attempt to determine whether the device exists or not, and abort the
    device creation if it doesn't.

http://bugzilla.kernel.org/show_bug.cgi?id=9614

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/glue.c     |   40 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/video.c    |    7 ++++++-
 include/acpi/acpi_bus.h |    2 ++
 3 files changed, 48 insertions(+), 1 deletion(-)

Index: linux-2.6.25/drivers/acpi/glue.c
===================================================================
--- linux-2.6.25.orig/drivers/acpi/glue.c
+++ linux-2.6.25/drivers/acpi/glue.c
@@ -140,6 +140,46 @@ struct device *acpi_get_physical_device(
 
 EXPORT_SYMBOL(acpi_get_physical_device);
 
+/* ToDo: When a PCI bridge is found, return the PCI device behind the bridge
+ *       This should work in general, but did not on a Lenovo T61 for the
+ *	 graphics card. But this must be fixed when the PCI device is
+ *       bound and the kernel device struct is attached to the acpi device
+ * Note: A success call will increase reference count by one
+ *       Do call put_device(dev) on the returned device then
+ */
+struct device *acpi_get_physical_pci_device(acpi_handle handle)
+{
+	struct device *dev;
+	long device_id;
+	acpi_status status;
+
+	status =
+		acpi_evaluate_integer(handle, "_ADR", NULL, &device_id);
+
+	if (ACPI_FAILURE(status))
+		return NULL;
+
+	/* We need to attempt to determine whether the _ADR refers to a
+	   PCI device or not. There's no terribly good way to do this,
+	   so the best we can hope for is to assume that there'll never
+	   be a device in the host bridge */
+	if (device_id >= 0x10000) {
+		/* It looks like a PCI device. Does it exist? */
+		dev = acpi_get_physical_device(handle);
+	} else {
+		/* It doesn't look like a PCI device. Does its parent
+		   exist? */
+		acpi_handle phandle;
+		if (acpi_get_parent(handle, &phandle))
+			return NULL;
+		dev = acpi_get_physical_device(phandle);
+	}
+	if (!dev)
+		return NULL;
+	return dev;
+}
+EXPORT_SYMBOL(acpi_get_physical_pci_device);
+
 static int acpi_bind_one(struct device *dev, acpi_handle handle)
 {
 	struct acpi_device *acpi_dev;
Index: linux-2.6.25/drivers/acpi/video.c
===================================================================
--- linux-2.6.25.orig/drivers/acpi/video.c
+++ linux-2.6.25/drivers/acpi/video.c
@@ -833,11 +833,16 @@ static void acpi_video_bus_find_cap(stru
 static int acpi_video_bus_check(struct acpi_video_bus *video)
 {
 	acpi_status status = -ENOENT;
-
+	struct device *dev;
 
 	if (!video)
 		return -EINVAL;
 
+	dev = acpi_get_physical_pci_device(video->device->handle);
+	if (!dev)
+		return -ENODEV;
+	put_device(dev);
+
 	/* Since there is no HID, CID and so on for VGA driver, we have
 	 * to check well known required nodes.
 	 */
Index: linux-2.6.25/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.25.orig/include/acpi/acpi_bus.h
+++ linux-2.6.25/include/acpi/acpi_bus.h
@@ -370,6 +370,8 @@ struct acpi_bus_type {
 int register_acpi_bus_type(struct acpi_bus_type *);
 int unregister_acpi_bus_type(struct acpi_bus_type *);
 struct device *acpi_get_physical_device(acpi_handle);
+struct device *acpi_get_physical_pci_device(acpi_handle);
+
 /* helper */
 acpi_handle acpi_get_child(acpi_handle, acpi_integer);
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int, unsigned int);

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Matthew Garrett]

On Thu, Jul 03, 2008 at 06:05:51PM +0200, Thomas Renninger wrote:
> Can these two go to linux-next or -mm, pls.

Not until the opregion code is merged, no. If you'd like that to happen 
faster, then figuring out why it triggers a 750ms delay on brightness 
changes on the T61 would be a great help, because I haven't a clue why 
it's broken...

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

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Thomas Renninger]

On Thursday 03 July 2008 18:08:22 Matthew Garrett wrote:
> On Thu, Jul 03, 2008 at 06:05:51PM +0200, Thomas Renninger wrote:
> > Can these two go to linux-next or -mm, pls.
>
> Not until the opregion code is merged, no.
Why?

> If you'd like that to happen 
> faster, then figuring out why it triggers a 750ms delay on brightness
> changes on the T61 would be a great help, because I haven't a clue why
> it's broken...

Try the next patch, it works for the T61.
These IGD parts could take quite a while still, while Toshibas and others
remain broken (and T61 poke on wrong hardware which could cause
all kind of badness).
The patch has an easy switch to allow video.ko to take over again
if IGD is detected and Linux is capable of it.

Please consider to take these two.

Thanks,

   Thomas

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Matthew Garrett]

On Thu, Jul 03, 2008 at 06:15:11PM +0200, Thomas Renninger wrote:
> On Thursday 03 July 2008 18:08:22 Matthew Garrett wrote:
> > On Thu, Jul 03, 2008 at 06:05:51PM +0200, Thomas Renninger wrote:
> > > Can these two go to linux-next or -mm, pls.
> >
> > Not until the opregion code is merged, no.
> Why?

Because it's entirely plausible that it'll break existing systems.

> Try the next patch, it works for the T61.
> These IGD parts could take quite a while still, while Toshibas and others
> remain broken (and T61 poke on wrong hardware which could cause
> all kind of badness).

The code's written and works, it just has a 750ms latency for reasons I 
don't understand.

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

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Yves-Alexis Perez]

On Thu, Jul 03, 2008 at 05:16:43PM +0100, Matthew Garrett wrote:
> > Try the next patch, it works for the T61.
> > These IGD parts could take quite a while still, while Toshibas and others
> > remain broken (and T61 poke on wrong hardware which could cause
> > all kind of badness).
> 
> The code's written and works, it just has a 750ms latency for reasons I 
> don't understand.

I dont know if it's the same thing, but I remember at one time that when
pressing my brightness keys (on a T61, intel graphics), the acpi event
appeared in /proc/acpi/event like 1 second later. I didn't check
precisely, but it could really be those 750ms. Irrc, this was only
happening after first suspend to ram.

Do these patches mean that, maybe in 2.6.27, brightness keys would work
directly from kernel, without needing hal, gnome-power-manager or
anything else, like X?

I can try those patches and report back, but on which kernel should I
apply them? 2.6.26-rc8? Or some git?

Cheers,
-- 
Yves-Alexis

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Matthew Garrett]

On Thu, Jul 03, 2008 at 06:21:34PM +0200, Yves-Alexis Perez wrote:

> I dont know if it's the same thing, but I remember at one time that when
> pressing my brightness keys (on a T61, intel graphics), the acpi event
> appeared in /proc/acpi/event like 1 second later. I didn't check
> precisely, but it could really be those 750ms. Irrc, this was only
> happening after first suspend to ram.

Yes, it's the same issue.

> Do these patches mean that, maybe in 2.6.27, brightness keys would work
> directly from kernel, without needing hal, gnome-power-manager or
> anything else, like X?

Right.

> I can try those patches and report back, but on which kernel should I
> apply them? 2.6.26-rc8? Or some git?

It's against drm git - search the dri-devel archives for the patch.

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

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Henrique de Moraes Holschuh]

Hi Matthew!

On Thu, 03 Jul 2008, Matthew Garrett wrote:

> On Thu, Jul 03, 2008 at 06:05:51PM +0200, Thomas Renninger wrote:
> > Can these two go to linux-next or -mm, pls.
> 
> Not until the opregion code is merged, no. If you'd like that to happen 
> faster, then figuring out why it triggers a 750ms delay on brightness 
> changes on the T61 would be a great help, because I haven't a clue why 
> it's broken...

I have never really tried finding out WTF in the firmware causes this, but:

1. The backlight-change firmware runs in SMM.
2. The calls happen in ACPI DSDT
3. The SMM code DOES take care of the CMOS NVRAM

You could well find something interesting if you dissect what happens to the
NVRAM.  Maybe changing it before/after/during the ACPI call has something to
do with it, etc.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Matthew Garrett]

On Fri, Jul 04, 2008 at 12:09:49AM -0300, Henrique de Moraes Holschuh wrote:
> On Thu, 03 Jul 2008, Matthew Garrett wrote:
> > Not until the opregion code is merged, no. If you'd like that to happen 
> > faster, then figuring out why it triggers a 750ms delay on brightness 
> > changes on the T61 would be a great help, because I haven't a clue why 
> > it's broken...
> 
> I have never really tried finding out WTF in the firmware causes this, but:
> 
> 1. The backlight-change firmware runs in SMM.
> 2. The calls happen in ACPI DSDT
> 3. The SMM code DOES take care of the CMOS NVRAM

As far as I can tell, it's sleeping in the DSDT. But all the ready flags 
are set appropriately according to the opregion spec, so I'm confused as 
to why. Hardware failure has meant I've got to use the T61 this week, so 
I'll try to fix it while I'm stuck in an airport tomorrow...

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

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Thomas Renninger]

On Thursday 03 July 2008 18:16:43 you wrote:
> On Thu, Jul 03, 2008 at 06:15:11PM +0200, Thomas Renninger wrote:
> > On Thursday 03 July 2008 18:08:22 Matthew Garrett wrote:
> > > On Thu, Jul 03, 2008 at 06:05:51PM +0200, Thomas Renninger wrote:
> > > > Can these two go to linux-next or -mm, pls.
> > >
> > > Not until the opregion code is merged, no.
> >
> > Why?
>
> Because it's entirely plausible that it'll break existing systems.
In fact it is fixing a lot systems. Toshiba and Lenovo are not the only one
declaring a dummy ACPI graphics device.

In fact only T61 can work by accident on the wrong graphics device (given
the fact that vendors use the same name for graphics devices, Toshiba at
least does this):

/* a hack to fix the duplicate name "VID" problem on T61 */
        if (!strcmp(device->pnp.bus_id, "VID")) {
                if (instance)
                        device->pnp.bus_id[3] = '0' + instance;
                instance ++;
        }

All others which init two graphics devices with the same name (like on 
Toshiba) already try to set up identical proc entries.

So it is not entirely plausible that it'll break existing systems, it is very 
unlikely that any other system than T61 break (given the fact I didn't 
introduce a bug, which is not that unlikely).

Hmm, there might be IGD devices that could just work with the default 
backlight functions...
Your hint in the other mail to check for tche flag is  a good one..
Also there should be a switch to provide both directions via boot param:
 - force to use video.ko
 - force to use vendor specific
-> will not acpi_vendor=backlight,display_switching boot param but:
acpi_display_output=video/vendor_specific
acpi_backlight=video/vendor_specific

> > Try the next patch, it works for the T61.
> > These IGD parts could take quite a while still, while Toshibas and others
> > remain broken (and T61 poke on wrong hardware which could cause
> > all kind of badness).
>
> The code's written and works, it just has a 750ms latency for reasons I
> don't understand.
We have a broken situation for 2 kernel version now.
It looks like it could take some more time and then it's quite likely that 
there are systems with other issues.

As soon as your parts are finished we can easily remove the
ACPI_VIDEO_IGD
and add these graphics devices to the one which should be served by your 
modifications.

My patch(es) provide the possibility to do backlight switching in the old 
fashion which always worked. A switch that provides:
 - force to use video.ko
 - force to use vendor specific
should definitely go in with or before your modifications, because they much 
more likely will break systems and people do want to have a switch then.

Please explain me again why this should not go in before/with your patches.
If we agree that this one (or a similar solution) makes sense I am going to 
post a 2nd version soon and then I can help a bit with the 750 ms issue...

Even we only see this with graphics cards right now, the check whether a PCI 
device exists for an ACPI device declaration, should probably go into a more 
general place later. When an ACPI driver registers and _STA is evaluated, 
acpi_get_physical_pci_device() should be invoked for pci devices without _STA 
function.

    Thomas

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Matthew Garrett]

On Fri, Jul 04, 2008 at 12:08:02PM +0200, Thomas Renninger wrote:

> Hmm, there might be IGD devices that could just work with the default 
> backlight functions...
> Your hint in the other mail to check for tche flag is  a good one..

There are IGD devices that just work with the default functions. I'm 
using one.

> As soon as your parts are finished we can easily remove the
> ACPI_VIDEO_IGD
> and add these graphics devices to the one which should be served by your 
> modifications.

As I said, your patches (in their current form) *will* break systems 
that currently work, and a quick check seems to indicate that that 
includes my machine. 

> Please explain me again why this should not go in before/with your patches.
> If we agree that this one (or a similar solution) makes sense I am going to 
> post a 2nd version soon and then I can help a bit with the 750 ms issue...

The first patch should go in with the opregion code. The second patch 
should go in without the IGD code. Can we please just fix this the right 
way?
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Henrique de Moraes Holschuh]

On Fri, 04 Jul 2008, Matthew Garrett wrote:
> On Fri, Jul 04, 2008 at 12:09:49AM -0300, Henrique de Moraes Holschuh wrote:
> > On Thu, 03 Jul 2008, Matthew Garrett wrote:
> > > Not until the opregion code is merged, no. If you'd like that to happen 
> > > faster, then figuring out why it triggers a 750ms delay on brightness 
> > > changes on the T61 would be a great help, because I haven't a clue why 
> > > it's broken...
> > 
> > I have never really tried finding out WTF in the firmware causes this, but:
> > 
> > 1. The backlight-change firmware runs in SMM.
> > 2. The calls happen in ACPI DSDT
> > 3. The SMM code DOES take care of the CMOS NVRAM
> 
> As far as I can tell, it's sleeping in the DSDT. But all the ready flags 

Which probably means the SMI trap was botched :(

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Matthew Garrett]

Got it. The reason for the delay is that BRTO sits in a loop for no 
obvious reason unless BRTF is cleared. The only thing that clears BRTF 
is the PWMS function, which isn't called anywhere in the ACPI tables. 
The NetBSD driver has the following code:

	/* Claim ownership of brightness control */
	param[0].Integer.Value = 0;
	(void)AcpiEvaluateObject(sc->sc_node->ad_handle, "PWMS", &params, NULL);

and doing this in thinkpad_acpi means my brightness control works in an 
acceptably speedy manner with opregion. Henrique, any thoughts on this? 
If you're happy with it, we can get opregion support into .27 and add 
the patch to remove duplicate ACPI video devices and everything will 
work in a beautiful and harmonious manner.

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index b596929..bbc45c8 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -899,6 +899,9 @@ static int __init tpacpi_check_std_acpi_brightness_support(void)
 
 	if (ACPI_SUCCESS(status) && bcl_levels > 2) {
 		tp_features.bright_acpimode = 1;
+		/* Set ACPI mode */
+		if (!acpi_evalf(hkey_handle, NULL, "PWMS", "vd", 0))
+			printk(TPACPI_INFO "Failed to claim backlight\n");
 		return (bcl_levels - 2);
 	}

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

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Henrique de Moraes Holschuh]

On Fri, 04 Jul 2008, Matthew Garrett wrote:
> Got it. The reason for the delay is that BRTO sits in a loop for no 
> obvious reason unless BRTF is cleared. The only thing that clears BRTF 
> is the PWMS function, which isn't called anywhere in the ACPI tables. 
> The NetBSD driver has the following code:
> 
> 	/* Claim ownership of brightness control */
> 	param[0].Integer.Value = 0;
> 	(void)AcpiEvaluateObject(sc->sc_node->ad_handle, "PWMS", &params, NULL);
> 
> and doing this in thinkpad_acpi means my brightness control works in an 
> acceptably speedy manner with opregion. Henrique, any thoughts on this? 
> If you're happy with it, we can get opregion support into .27 and add 
> the patch to remove duplicate ACPI video devices and everything will 
> work in a beautiful and harmonious manner.

Well done!

Please give me one or two days to check the data I have and look over the
new DSDTs, and I will give you a better reply.

I really should go over the FreeBSD and NetBSD thinkpad drivers with a fine
comb looking over for stuff they figured out about the firmware that
thinkpad-acpi doesn't know about yet.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 1/2] ACPI Video: Ignore devices that aren't present in hardware

[Henrique de Moraes Holschuh]

On Fri, 04 Jul 2008, Matthew Garrett wrote:
> Got it. The reason for the delay is that BRTO sits in a loop for no 
> obvious reason unless BRTF is cleared. The only thing that clears BRTF 
> is the PWMS function, which isn't called anywhere in the ACPI tables. 
> The NetBSD driver has the following code:

PWMS: missing from X300, X61t, X61... is this a new thing that Lenovo
recently added to the firmware?  I don't have very new dumps, particularly,
none taken without OSI(Linux) -- might do something to the SSDTs -- so I
might not have the full view of this.

Anyway, if it is that new AND we need it to work well, it means we need to
warn the user to upgrade the firmware when it is missing (and of course, add
your code to use it when it is not missing :p).

BRTO, BRTF: these are around for a while already.  But I can't find anything
that calls BRTO.  What is NetBSD doing with BRTO?

Can you send me your acpidump (and censored dmidecode) output so that I can
look at a new enough dump with PWMS support?

> and doing this in thinkpad_acpi means my brightness control works in an 
> acceptably speedy manner with opregion. Henrique, any thoughts on this? 

I'd like to have newer versions of the tables, before giving an opinion. Can
you send me the dump for your thinkpad?

> If you're happy with it, we can get opregion support into .27 and add 
> the patch to remove duplicate ACPI video devices and everything will 
> work in a beautiful and harmonious manner.

I didn't look much at the code for removing duplicate ACPI video yet, but I
am fine with the basic idea.

However, if we are going to be disabling the entire firmware brightness
change system (which will include ACPI BCM, probably... need the tables to
know), that's something the X server needs to be able to enable/disable.
Otherwise, we break all such changes outside of a X display, which is not
acceptable.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh