[PATCH] Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it

[PATCH] Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it

[Thomas Renninger]

Hi Matthew,

I posted this one already and you had some concerns about it:
http://www.mail-archive.com/linux-acpi@vger.kernel.org/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

Signed-off-by: Thomas Renninger <trenn@suse.de>

---
 drivers/acpi/video.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -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 acp
 	struct 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)

Re: [PATCH] Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it

[Thomas Renninger]

On Fri, 2007-12-07 at 13:55 +0100, Thomas Renninger wrote:
> Hi Matthew,
> 
> I posted this one already and you had some concerns about it:
> http://www.mail-archive.com/linux-acpi@vger.kernel.org/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).

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

> What do you think?
> 
> ---------------
> 
> 
> Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> 
> ---
>  drivers/acpi/video.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> Index: linux-2.6/drivers/acpi/video.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/video.c
> +++ linux-2.6/drivers/acpi/video.c
> @@ -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 acp
>  	struct 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it

[Matthew Garrett]

On Fri, Dec 07, 2007 at 01:55:13PM +0100, Thomas Renninger wrote:
> Hi Matthew,
> 
> I posted this one already and you had some concerns about it:
> http://www.mail-archive.com/linux-acpi@vger.kernel.org/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

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.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it

[Thomas Renninger]

On Fri, 2007-12-07 at 22:50 +0000, Matthew Garrett wrote:
> On Fri, Dec 07, 2007 at 01:55:13PM +0100, Thomas Renninger wrote:
> > Hi Matthew,
> > 
> > I posted this one already and you had some concerns about it:
> > http://www.mail-archive.com/linux-acpi@vger.kernel.org/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
> 
> 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.
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.

> 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

> 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.

> 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.
> 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.

> 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

Re: [PATCH] Recent Lenovo ThinkPads define a dummy grahpics device, find it and ignore it

[Matthew Garrett]

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.

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

[PATCH] Ignore ACPI video devices that aren't present in hardware

[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 <mjg59@srcf.ucam.org>

---

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.

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 12b2adb..1906eff 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -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);

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

Re: [PATCH] Ignore ACPI video devices that aren't present in hardware

[Thomas Renninger]

On Thu, 2008-01-17 at 03:39 +0000, Matthew Garrett wrote:
> 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 <mjg59@srcf.ucam.org>
> 
> ---
> 
> 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.
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 12b2adb..1906eff 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -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);

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

[Matthew Garrett]

On Thu, Jan 17, 2008 at 11:57:11AM +0100, Thomas Renninger wrote:

> 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.

> 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.

> 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.

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

Re: [PATCH] Ignore ACPI video devices that aren't present in hardware

[Zhang Rui]

On Thu, 2008-01-17 at 20:02 +0800, Matthew Garrett wrote:
> On Thu, Jan 17, 2008 at 11:57:11AM +0100, Thomas Renninger wrote:
> 
> > 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.
> 
> > 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.
> 
> > 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.
> 
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

[Julian Sikorski]

Zhang Rui pisze:
> On Thu, 2008-01-17 at 20:02 +0800, Matthew Garrett wrote:
>> On Thu, Jan 17, 2008 at 11:57:11AM +0100, Thomas Renninger wrote:
>>
>>> 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.
>>
>>> 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.
>>
>>> 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.
>>
> 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
> 
I tested the patch and it works for me. Some more details in the bug. 
Thank you very much for fixing that.

Regards,
Julian

Re: [PATCH] Ignore ACPI video devices that aren't present in hardware

[Len Brown]

On Wednesday 16 January 2008 22:39, Matthew Garrett wrote:
> 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 <mjg59@srcf.ucam.org>
> 
> ---
> 
> 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.
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 12b2adb..1906eff 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -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 */

_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

> +	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

[Matthew Garrett]

On Thu, Jan 24, 2008 at 05:21:17PM -0500, Len Brown wrote:
> _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

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.

> 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.

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.

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

Re: [PATCH] Ignore ACPI video devices that aren't present in hardware

[Len Brown]

On Saturday 26 January 2008 21:09, Matthew Garrett wrote:
> On Thu, Jan 24, 2008 at 05:21:17PM -0500, Len Brown wrote:
> > _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
> 
> 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.

oh, ic, you're right.

> 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.

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

[Matthew Garrett]

On Sat, Feb 02, 2008 at 02:12:17AM -0500, Len Brown wrote:

> 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()?

Yeah, that's fair enough. I'll respin it once I'm on the ground for more 
than 3 hours.

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

Re: [PATCH] Ignore ACPI video devices that aren't present in hardware

[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 <mjg59@srcf.ucam.org>
---

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 09a85eb..11c8335 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -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.
 	 */

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

Re: [PATCH] Ignore ACPI video devices that aren't present in hardware

[Len Brown]

thanks for the update, matthew.

applied,
-len

On Wednesday 06 February 2008 20:44, Matthew Garrett wrote:
> 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 <mjg59@srcf.ucam.org>
> ---
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 09a85eb..11c8335 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -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.
>  	 */
>