Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it
PATCH 1/1 Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it
Hi Matthew,
I posted this one already and you had some concerns about it:
http://www.mail-archive.com/[email protected]/msg09510.html
IMO, this one should still be added to -mm, because:
- The patch itself is correct, it is the function that is called that must make sure to return the corresponding pci device
- There currently is a bug: Lenovos will register a device for which no physical device exists.
- That means on these machines HW is addressed/poked
which does not exist -> danger.
- There is no other way to fix up acpi_get_physical_device, than to just do this change (at least I don't see it). People have to complain that their device is not found (the message they see in dmesg is obvious).
Then we can make acpi_get_physical_device more robust, which will come out as a benefit for other functionality too sooner or later to be able to rely on getting struct pci in the video (and possibly other) driver(s).
What do you think?
---------------
Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it
--- drivers/acpi/video.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/acpi/video.c +16 -0
@@ -35,6 +35,7 @@#include <linux/input.h> #include <linux/backlight.h> #include <linux/video_output.h> +#include <linux/acpi.h> #include <asm/uaccess.h> #include <acpi/acpi_bus.h>
@@ -1896,6 +1897,21 @@ static int acpi_video_bus_add(struct acpstruct acpi_video_bus *video;struct input_dev *input;int error; +struct device *dev; + + +/* +* Check whether we have really a graphics device physically +* in the slot and registered at the system. +*/ + dev =acpi_get_physical_device (device->handle); +if (!dev) { + printk (KERN_DEBUG PREFIX "Video device %s.%s not physically" +" connected, ignoring\n" ,acpi_device_bid (device), + device->parent ? acpi_device_bid(device->parent) : ""); +return -ENODEV ; + } +put_device (dev); video =kzalloc (sizeof (struct acpi_video_bus ),GFP_KERNEL );if (!video)
Quoted 15 lines · Thomas Renninger
Hi Matthew,
I posted this one already and you had some concerns about it:
http://www.mail-archive.com/[email protected]/msg09510.htmlIMO, this one should still be added to -mm, because:
- The patch itself is correct, it is the function that is called that must make sure to return the corresponding pci device- There currently is a bug: Lenovos will register a device for which no physical device exists.
- That means on these machines HW is addressed/poked
which does not exist -> danger.
Quoted 8 lines
- There is no other way to fix up
acpi_get_physical_device, than to just do this change (at least I don't see it). People have to complain that their device is not found (the message they see in dmesg is obvious).
Then we can makeacpi_get_physical_devicemore robust, which will come out as a benefit for other functionality too sooner or later to be able to rely on getting struct pci in the video (and possibly other) driver(s).
I forgot to mention, that not only Lenovos are affected.
I tested this on an HP Compaq 6715b and the video device is still found. I also tested this on a Toshiba Satellite A210 and there the patch is also mandatory!
There two VGA devices are declared, which results in /proc/acpi/video showing *two* "VGA" device entries in the same proc directory. Here the hack (drivers/acpi/video.c):
/* 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 ++;
}which was written for duplicated Lenovo devices (where BIOS names the graphics device "VID") does not help.
Brightness switching is broken there without this patch and works fine with it.
Please add it to mm.
Is that ok with you, Matthew?
Thomas
Quoted 53 lines
What do you think?
---------------
Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it
Signed-off-by Thomas Renninger <[email protected]>--- drivers/acpi/video.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)Index:
linux-2.6/drivers/acpi/video.c
===================================================================drivers/acpi/video.c +16
-1@@ -35,6 +35,7 @@#include <linux/input.h> #include <linux/backlight.h> #include <linux/video_output.h> +#include <linux/acpi.h> #include <asm/uaccess.h> #include <acpi/acpi_bus.h> @@ -1896,6 +1897,21 @@ static int acpi_video_bus_add(struct acpstruct acpi_video_bus *video;struct input_dev *input;int error; +struct device *dev; + + +/* +* Check whether we have really a graphics device physically +* in the slot and registered at the system. +*/ + dev =acpi_get_physical_device (device->handle); +if (!dev) { + printk (KERN_DEBUG PREFIX "Video device %s.%s not physically" +" connected, ignoring\n" ,acpi_device_bid (device), + device->parent ? acpi_device_bid(device->parent) : ""); +return -ENODEV ; + } +put_device (dev); video =kzalloc (sizeof (struct acpi_video_bus ),GFP_KERNEL );if (!video) -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
Quoted 8 lines · Thomas Renninger
Hi Matthew,
I posted this one already and you had some concerns about it:
http://www.mail-archive.com/[email protected]/msg09510.htmlIMO, this one should still be added to -mm, because:
- The patch itself is correct, it is the function that is called that must make sure to return the corresponding pci device
No, I don't think that follows. The semantics of acpi_get_physical_device are currently to return a physical device only if the node directly corresponds to one. What's the correct physical device for the video extension? It may be a PCI device, but it's just as easy to argue that it corresponds to some piece of platform-specific hardware.
We could change the semantics and ensure that all leaf nodes of an ACPI node representing a PCI device share the same physical device. That would avoid the bug your patch currently introduces of ignoring functional devices, but would immediately cause the second video device to reappear on Thinkpads.
Incidentally, have you tried this on a Thinkpad with a discrete graphics controller? I /suspect/ (but can't verify) that you'll end up discarding the working video extension.
In summary, I don't think this approach can be made to work. You're throwing out legitimate and working devices. Instead, we should export information about the addresses of the video extensions and let whoever's handling the graphics (which is userspace right now) handle it.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 10 lines · Matthew Garrett
Quoted 8 lines · Thomas Renninger
Hi Matthew,
I posted this one already and you had some concerns about it:
http://www.mail-archive.com/[email protected]/msg09510.htmlIMO, this one should still be added to -mm, because:
- The patch itself is correct, it is the function that is called that must make sure to return the corresponding pci device
Quoted 6 lines
No, I don't think that follows. The semantics of
acpi_get_physical_deviceare currently to return a physical device only if the node directly corresponds to one. What's the correct physical device for the video extension? It may be a PCI device, but it's just as easy to argue that it corresponds to some piece of platform-specific hardware.
What do you mean with "platform-specific" hardware?
A graphics card on an ACPI capable system should always be PCI, PCIe or AGP driven and a call to acpi_get_physical_device should return its struct pci structure. AFAIK it may return the bridge device atm, this could be fixed up later, in this case it does not matter as no values of the PCI device are needed, it's only needed to check whether it exists at all and if the bridge exists it's enough.
Quoted 5 lines
We could change the semantics and ensure that all leaf nodes of an ACPI node representing a PCI device share the same physical device. That would avoid the bug your patch currently introduces of ignoring functional devices, but would immediately cause the second video device to reappear on Thinkpads.
I thought it is done that way?
You mean e.g. going up to the PCI bus object and get the bus value, then get func and slot from the child on which branch our device sits:
PCI0 (bus0)
/ \
XY (slot 2, func1)
/ | \
A B C
You mean here device A, B and C should all return the pci device at 0:2.1, this should be the case?
It at least was with the "find pci device" func I came up with my very first patch, before you pointed me to: acpi_get_physical_device
This would also work for ThinkPads:
PCI0 (bus0)
/ \
VID AGP
(slot2, func0) (slot1, func0)
|
VID
(ADR 0x00)Slot, func no.s are made up.
0:1.0 would point to a PCI-PCI bridge, which only exists on external/
discrete graphics card (with own mem etc.) acpi_get_physical_device could get extended to point to the real graphics pci device behind the bridge
0:2.0 would point to the internal graphics card sitting on a PCIe bus
Quoted 3 lines
Incidentally, have you tried this on a Thinkpad with a discrete graphics controller? I /suspect/ (but can't verify) that you'll end up discarding the working video extension.
No, it should work, see above.
Brightness switching may not work because the current ACPI implementation requires a new graphics card driver feature which should be supported by the latest Intel driver (or at least soon). Anyway it is the specified mechanism how things should work.
Quoted 1 line
In summary, I don't think this approach can be made to work.
You cannot check in kernel whether an ACPI graphics device exists, but you can in userspace? I doubt that.
Quoted 2 lines
You're
throwing out legitimate and working devices.
Please prove. I tested this on three totally different machines:
- Lenovo
- Toshiba
- HP
Only recent Vista capable systems should need this, that means those tests cover a reasonable amount of all video extension supporting machines.
Quoted 4 lines
Instead, we should export
information about the addresses of the video extensions and let whoever's handling the graphics (which is userspace right now) handle it.
No, this is wrong.
Please explain how this should work more detailed, a solution is needed now.
Currently the video driver is severely broken for a lot machines (at least Lenovo and Toshiba). Severely means that HW registers are accessed for which no device exist. E.g. if you hit the brightness up/down button on a Lenovo ThinkPad, it may happen that the wrong device brightness functions are served (this is why you may see brightness switching working with an internal Intel graphics card, because the Nvidia/ATI external device's brightness functions are processed which also works with the other device by luck).
On the Toshiba, brightness switching does not work (jumps irregularly) with both devices registered and it's again luck that the machine is simply not freezing at all.
Thomas
I've had more time to look at this now. Let's take one of the most pathological cases, the Dell d610. It's available with either Intel or ATI graphics. The DSDT declares three video devices. Two are called VID, the third is called VID2. I'll call the first two VID(a) and VID(b) to reduce confusion.
The layout is as follows:
host bridge (_ADR 0x00)
|
|
/ \
/ \
AGP (_ADR 0x00010000) |
| / \
| / \VID(a) (_ADR 0x00) / \
VID(b) (_ADR 0x00020000) VID2 (_ADR 0x00020001)
If we call acpi_get_physical_device on VID(a), we'll get nothing - there isn't a corresponding physical device. Therefore we have to call it on the parent, and so will get the AGP bridge if the machine is configured that way. If not, we can be sure that VID(a) doesn't exist.
If we call acpi_get_physical_device on VID(b), then either the device exists and we get something, or the device doesn't exist and we don't. In the second case, how do we distinguish between that being because the device doesn't exist and that being because the physical device is the parent? If we call acpi_get_physical_device on the parent of VID(b), then we'll hit the host bridge and we'll get a PCI device back. This then looks almost identical to the AGP bridge case.
There are two things we can do here. The first is to examine the _ADR field and use that to determine whether we should look at the parent or not (if it's parsable as a PCI address, look at the current device. If not, look at the parent device). The second is to always look at the parent device and ignore it if the host bridge is returned.
I think the first approach is probably better, since it doesn't result in us depending on the vagueries of bus topology. However, I'm concerned about the inherent assumption it makes about video devices being PCI-like. That's true now, but I'm aware of at least one forthcoming platform where that may not be the case. The spec certainly doesn't require it. I'll try to come up with a patch in any case.
Signature · 1 line
Matthew Garrett | [email protected]
PATCH 1/1 Ignore ACPI video devices that aren't present in hardware
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.
---
I'm still not convinced that this can be done reliably, especially if anyone ever produces ACPI devices that aren't PCI-based. On the other hand, I believe that this will work correctly (ie, discard devices for integrated hardware if discrete hardware is present, and vice-versa) in all existing cases. The most irritating feature is that there's no good way to determine if an _ADR method refers to a PCI device or not - 0x00 could mean the PCI host bridge or the first video device hanging off an AGP bridge. So far all the implementations I've seen use low numbers for devices that aren't directly on the root PCI bus, so I've just assumed that they'll always be less than 0x10000. If someone puts a graphics core in their host bridge, this will break. But that would seem a mad thing to do, so I'm going to pretend it's impossible.
drivers/acpi/video.c +29 -0
@@ -1266,8 +1266,37 @@ acpi_video_bus_write_DOS(struct file *file,static int acpi_video_bus_add_fs (struct acpi_device *device) { +long device_id; +int status;struct proc_dir_entry *entry =NULL ;struct acpi_video_bus *video; +struct device *dev; + + status = +acpi_evaluate_integer (device->handle,"_ADR" ,NULL , &device_id); + +if (!ACPI_SUCCESS (status)) +return -ENODEV ; + +/* 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 video device in the host bridge */ +if (device_id >=0x10000 ) { +/* It looks like a PCI device. Does it exist? */ + dev =acpi_get_physical_device (device->handle); + }else { +/* It doesn't look like a PCI device. Does its parent +exist? */ +acpi_handle phandle; +if (acpi_get_parent (device->handle, &phandle)) +return -ENODEV ; + dev =acpi_get_physical_device (phandle); + } +if (!dev) +return -ENODEV ; +put_device (dev); + video =acpi_driver_data (device);
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
Quoted 55 lines · 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.
Signed-off-by Matthew Garrett <[email protected]>---
I'm still not convinced that this can be done reliably, especially if anyone ever produces ACPI devices that aren't PCI-based. On the other hand, I believe that this will work correctly (ie, discard devices for integrated hardware if discrete hardware is present, and vice-versa) in all existing cases. The most irritating feature is that there's no good way to determine if an
_ADRmethod refers to a PCI device or not -0x00could mean the PCI host bridge or the first video device hanging off an AGP bridge. So far all the implementations I've seen use low numbers for devices that aren't directly on the root PCI bus, so I've just assumed that they'll always be less than0x10000. If someone puts a graphics core in their host bridge, this will break. But that would seem a mad thing to do, so I'm going to pretend it's impossible.drivers/acpi/video.c +22
-0@@ -1266,8 +1266,37 @@ acpi_video_bus_write_DOS(struct file *file,static int acpi_video_bus_add_fs (struct acpi_device *device) { +long device_id; +int status;struct proc_dir_entry *entry =NULL ;struct acpi_video_bus *video; +struct device *dev; + + status = +acpi_evaluate_integer (device->handle,"_ADR" ,NULL , &device_id); + +if (!ACPI_SUCCESS (status)) +return -ENODEV ; + +/* 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 video device in the host bridge */ +if (device_id >=0x10000 ) { +/* It looks like a PCI device. Does it exist? */ + dev =acpi_get_physical_device (device->handle); + }else { +/* It doesn't look like a PCI device. Does its parent +exist? */ +acpi_handle phandle; +if (acpi_get_parent (device->handle, &phandle))
drivers/acpi/video.c acpi_video_bus_write_DOS(struct file *file, · line 1302
Quoted 10 lines
drivers/acpi/video.c +7
-0+return -ENODEV ; + dev =acpi_get_physical_device (phandle); + } +if (!dev) +return -ENODEV ; +put_device (dev); + video =acpi_driver_data (device);
First, thanks for looking at this.
Does it make sense to add this as a separate function, searching for a physical PCI device for an ACPI device may pop up more often in the future? This is a kind of _STA (present or not) function for PCI ACPI devices then.
I also wonder why you added this to acpi_video_bus_add_fs and not acpi_video_bus_add, but it's functionally the same.
This has been tested on a Dell 610 only then?
This sounds like an older machine, I wonder whether the video driver is useful on this one at all and whether Windows had to check whether the devices are present also...
What does the video device provide in functionality on this older Dell? Vista requires the ACPI graphics devices for some functionalities. The video device has not been used much (this is at least correct for SUSE, AFAIK also for all other distributions, the video driver was in a rather bad shape all the time?). I have no problem to blacklist the driver for BIOSes older than VISTA capables ones.
We won't loose any functionality, but may prevent some problems when we do not release the video driver on older machines.
I don't know exactly how these ACPI BIOSes are made up, but it looks like there are components added by each HW vendor adding some HW to the board. So you might have some graphics devices in the old BIOSes, but the real functionality might have been implemented in an Asus, Sony, IBM, whatever vendor specific ACPI device or a native driver. Now for VISTA the vendor has to test that these ACPI graphics devices are really functional (this might not have been the case before) as they will get used by the OS.
I would not take the Dell 610 as a reference system here.
Please tell me if you have tested more machines, I try to give it a test on a Toshiba, Lenovo and whatever I find with a Vista capable BIOS (IMO this should be the most important criterion for finding the devices, I doubt older Windows Versions made much use of ACPI graphics devices) as soon as I find some time...
Thanks,
Thomas
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
Quoted 4 lines · Thomas Renninger
Does it make sense to add this as a separate function, searching for a physical PCI device for an ACPI device may pop up more often in the future? This is a kind of
_STA(present or not) function for PCI ACPI devices then.
No, I don't think this can be done generically - _ADR for a SATA device has a format that looks similar to the one for PCI devices, for instance. I think you need to have knowledge of the specific device type.
Quoted 4 lines
This has been tested on a Dell 610 only then?
This sounds like an older machine, I wonder whether the video driver is useful on this one at all and whether Windows had to check whether the devices are present also...
Also on an HP 2510p. I suspect that the Windows behaviour is to leave this up to the graphics driver rather than handling it in the core. Toshiba seem to have implemented the complete spec for some time, but several other vendors only implemented the section for display output switching and obtaining the EDID.
Quoted 5 lines
Please tell me if you have tested more machines, I try to give it a test on a Toshiba, Lenovo and whatever I find with a Vista capable BIOS (IMO this should be the most important criterion for finding the devices, I doubt older Windows Versions made much use of ACPI graphics devices) as soon as I find some time...
Yes, I've tested it on some other machines - the HP is the only one I've tested with a designed for Vista badge, but it should be correct in these cases as well.
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
Quoted 34 lines · Matthew Garrett
Quoted 1 line · Thomas Renninger
Does it make sense to add this as a separate function, searching for
a
Quoted 2 lines
physical PCI device for an ACPI device may pop up more often in the future? This is a kind of
_STA(present or not) function for PCIACPI
Quoted 1 line
devices then.
No, I don't think this can be done generically -
_ADRfor a SATA device
has a format that looks similar to the one for PCI devices, for instance. I think you need to have knowledge of the specific device type.Quoted 2 lines
This has been tested on a Dell 610 only then?
This sounds like an older machine, I wonder whether the video driveris
Quoted 1 line
useful on this one at all and whether Windows had to check whether
the
Quoted 1 line
devices are present also...
Also on an HP 2510p. I suspect that the Windows behaviour is to leave this up to the graphics driver rather than handling it in the core. Toshiba seem to have implemented the complete spec for some time, but several other vendors only implemented the section for display output switching and obtaining the EDID.
Quoted 1 line
Please tell me if you have tested more machines, I try to give it a
test
Quoted 1 line
on a Toshiba, Lenovo and whatever I find with a Vista capable BIOS
(IMO
Quoted 1 line
this should be the most important criterion for finding the devices,
I
Quoted 9 lines
Quoted 1 line
doubt older Windows Versions made much use of ACPI graphics devices)
as
Quoted 1 line
soon as I find some time...
Yes, I've tested it on some other machines - the HP is the only one I've
tested with a designed for Vista badge, but it should be correct in these cases as well.
I've tested it on a T61 and it works.
I asked Julian to test it on a Toshiba Satellite A100-87. Please refer to http://bugzilla.kernel.org/show_bug.cgi?id=9614
Thanks,
Rui
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
Zhang Rui pisze:
Quoted 41 lines · Matthew Garrett
Quoted 31 lines · Thomas Renninger
Quoted 2 lines
Does it make sense to add this as a separate function, searching for
a
Quoted 2 lines
physical PCI device for an ACPI device may pop up more often in the future? This is a kind of
_STA(present or not) function for PCIACPI
Quoted 1 line
devices then.
No, I don't think this can be done generically -
_ADRfor a SATA device
has a format that looks similar to the one for PCI devices, for instance. I think you need to have knowledge of the specific device type.Quoted 2 lines
This has been tested on a Dell 610 only then?
This sounds like an older machine, I wonder whether the video driveris
Quoted 1 line
useful on this one at all and whether Windows had to check whether
the
Quoted 1 line
devices are present also...
Also on an HP 2510p. I suspect that the Windows behaviour is to leave this up to the graphics driver rather than handling it in the core. Toshiba seem to have implemented the complete spec for some time, but several other vendors only implemented the section for display output switching and obtaining the EDID.
Quoted 1 line
Please tell me if you have tested more machines, I try to give it a
test
Quoted 1 line
on a Toshiba, Lenovo and whatever I find with a Vista capable BIOS
(IMO
Quoted 1 line
this should be the most important criterion for finding the devices,
Quoted 9 lines
I
Quoted 1 line
doubt older Windows Versions made much use of ACPI graphics devices)
as
Quoted 1 line
soon as I find some time...
Yes, I've tested it on some other machines - the HP is the only one I've
tested with a designed for Vista badge, but it should be correct in these cases as well.
Quoted 7 lines
I've tested it on a T61 and it works.
I asked Julian to test it on a Toshiba Satellite A100-87. Please refer to http://bugzilla.kernel.org/show_bug.cgi?id=9614Thanks,
Rui
I tested the patch and it works for me. Some more details in the bug. Thank you very much for fixing that.
Regards,
Julian
Len Brown
reviewerRe: [PATCH] Ignore ACPI video devices that aren't present in hardware
In reply to Matthew GarrettQuoted 37 lines · 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.
Signed-off-by Matthew Garrett <[email protected]>---
I'm still not convinced that this can be done reliably, especially if anyone ever produces ACPI devices that aren't PCI-based. On the other hand, I believe that this will work correctly (ie, discard devices for integrated hardware if discrete hardware is present, and vice-versa) in all existing cases. The most irritating feature is that there's no good way to determine if an
_ADRmethod refers to a PCI device or not -0x00could mean the PCI host bridge or the first video device hanging off an AGP bridge. So far all the implementations I've seen use low numbers for devices that aren't directly on the root PCI bus, so I've just assumed that they'll always be less than0x10000. If someone puts a graphics core in their host bridge, this will break. But that would seem a mad thing to do, so I'm going to pretend it's impossible.drivers/acpi/video.c +4
-0@@ -1266,8 +1266,37 @@ acpi_video_bus_write_DOS(struct file *file,static int acpi_video_bus_add_fs (struct acpi_device *device) { +long device_id; +int status;struct proc_dir_entry *entry =NULL ;struct acpi_video_bus *video; +struct device *dev; +
drivers/acpi/video.c acpi_video_bus_write_DOS(struct file *file, · line 1284
Quoted 10 lines
drivers/acpi/video.c +10
-0+ status = +acpi_evaluate_integer (device->handle,"_ADR" ,NULL , &device_id); + +if (!ACPI_SUCCESS (status)) +return -ENODEV ; + +/* 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 video device in the host bridge */
_ADR returns a bus-specific format.
In the case of a video device, section B.6.1 says that _ADR returns a 32-bit device ID, and that
it is the same number as returned in the _DOD list.
That is the only constraint.
So I don't think you can use the numerical value returned to have any particular meaning at all.
In particular, comparing it to magic number 0x10000 makes no sense.
-Len
Quoted 19 lines
drivers/acpi/video.c +15
-0+if (device_id >=0x10000 ) { +/* It looks like a PCI device. Does it exist? */ + dev =acpi_get_physical_device (device->handle); + }else { +/* It doesn't look like a PCI device. Does its parent +exist? */ +acpi_handle phandle; +if (acpi_get_parent (device->handle, &phandle)) +return -ENODEV ; + dev =acpi_get_physical_device (phandle); + } +if (!dev) +return -ENODEV ; +put_device (dev); + video =acpi_driver_data (device);
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
Quoted 3 lines · Len Brown
_ADRreturns a bus-specific format.
In the case of a video device, section B.6.1 says that_ADRreturns a 32-bit device ID, and that
No, that's only true for the output devices - not the parent device. The parent device is what's interesting here, and the format of its _ADR method is not defined.
Quoted 3 lines
So I don't think you can use the numerical value returned to have any particular meaning at all.
In particular, comparing it to magic number0x10000makes no sense.
In practice, it's either going to be a PCI device ID or something custom. If it's custom, it's unlikely to be a PCI device ID. From that point of view, 0x10000 isn't magic - it's the lowest PCI device ID that stands a real chance of being the graphics hardware or bridge that the graphics hardware is attached to.
Signature · 1 line
Matthew Garrett | [email protected]
Len Brown
reviewerRe: [PATCH] Ignore ACPI video devices that aren't present in hardware
Quoted 8 lines · Matthew Garrett
Quoted 3 lines · Len Brown
_ADRreturns a bus-specific format.
In the case of a video device, section B.6.1 says that_ADRreturns a 32-bit device ID, and thatNo, that's only true for the output devices - not the parent device. The parent device is what's interesting here, and the format of its
_ADRmethod is not defined.
oh, ic, you're right.
Quoted 5 lines
In practice, it's either going to be a PCI device ID or something custom. If it's custom, it's unlikely to be a PCI device ID. From that point of view,
0x10000isn't magic - it's the lowest PCI device ID that stands a real chance of being the graphics hardware or bridge that the graphics hardware is attached to.
I share your uneasyness about this heuristic, but I don't have a better idea
and this seems to work, so I think we should proceed...
But I have to agree with Thomas' feedback:
"I also wonder why you added this to acpi_video_bus_add_fs and not
acpi_video_bus_add, but it's functionally the same."
or perhaps acpi_video_bus_check()?
I'll lob this into the test tree in the mean-time...
thanks,
-Len
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
Quoted 5 lines · Len Brown
But I have to agree with Thomas' feedback:
"I also wonder why you added this toacpi_video_bus_add_fsand not
acpi_video_bus_add, but it's functionally the same."or perhaps
acpi_video_bus_check()?
Yeah, that's fair enough. I'll respin it once I'm on the ground for more than 3 hours.
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
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
---
drivers/acpi/video.c +30 -1
@@ -735,11 +735,40 @@ static void acpi_video_bus_find_cap(struct acpi_video_bus *video)static int acpi_video_bus_check (struct acpi_video_bus *video) {acpi_status status = -ENOENT ; - +long device_id; +struct device *dev; +struct acpi_device *device;if (!video)return -EINVAL ; + device = video->device; + + status = +acpi_evaluate_integer (device->handle,"_ADR" ,NULL , &device_id); + +if (!ACPI_SUCCESS (status)) +return -ENODEV ; + +/* 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 video device in the host bridge */ +if (device_id >=0x10000 ) { +/* It looks like a PCI device. Does it exist? */ + dev =acpi_get_physical_device (device->handle); + }else { +/* It doesn't look like a PCI device. Does its parent +exist? */ +acpi_handle phandle; +if (acpi_get_parent (device->handle, &phandle)) +return -ENODEV ; + dev =acpi_get_physical_device (phandle); + } +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. */
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] Ignore ACPI video devices that aren't present in hardware
thanks for the update, matthew.
applied,
-len
Quoted 56 lines · 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'tSigned-off-by Matthew Garrett <[email protected]>---
drivers/acpi/video.c +30
-1@@ -735,11 +735,40 @@ static void acpi_video_bus_find_cap(struct acpi_video_bus *video)static int acpi_video_bus_check (struct acpi_video_bus *video) {acpi_status status = -ENOENT ; - +long device_id; +struct device *dev; +struct acpi_device *device;if (!video)return -EINVAL ; + device = video->device; + + status = +acpi_evaluate_integer (device->handle,"_ADR" ,NULL , &device_id); + +if (!ACPI_SUCCESS (status)) +return -ENODEV ; + +/* 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 video device in the host bridge */ +if (device_id >=0x10000 ) { +/* It looks like a PCI device. Does it exist? */ + dev =acpi_get_physical_device (device->handle); + }else { +/* It doesn't look like a PCI device. Does its parent +exist? */ +acpi_handle phandle; +if (acpi_get_parent (device->handle, &phandle)) +return -ENODEV ; + dev =acpi_get_physical_device (phandle); + } +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. */