| Message ID | 1247649488.26272.106.camel@rzhang-dt (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, 15 Jul 2009, Zhang Rui wrote: > Only one ACPI video bus device for a VGA controller. > > Some buggy BIOS exports multiple ACPI video bus devices for the same > VGA controller, and multiple backlight control methods as well. > This messes up the ACPI video backlight control. > http://bugzilla.kernel.org/show_bug.cgi?id=13577 > > With this patch applied, only the first ACPI video bus device > under a PCI device node is bind to ACPI video driver. > > The questions is that, we never notice this kind of devices before, > thus I'm not sure this won't break any laptops. Laptops often have more than one video bus device, and you _have_ to choose the one that is _active_ (which might not be the first one you find). This is done on laptops that can have either discrete or in-chipset graphics, for example. I have seen it in several thinkpad models. There is also the new class of laptops that have *both* discrete and in-chipset GPUs and can switch them at runtime (well, Linux can't cope with it yet and ends up requiring the switch to be done in BIOS setup screen, AFAIK, but I hope that changes someday). I never looked at the DSDT of one of these to know how it looks like, but it would make sense that the ACPI video driver would have to be able to bind to more than one display device, and drive them separately, and do the right thing when the device is hotplugged/hotunplugged (or activated/deactivated)...
Henrique de Moraes Holschuh wrote: > On Wed, 15 Jul 2009, Zhang Rui wrote: >> Only one ACPI video bus device for a VGA controller. >> >> Some buggy BIOS exports multiple ACPI video bus devices for the same >> VGA controller, and multiple backlight control methods as well. >> This messes up the ACPI video backlight control. >> http://bugzilla.kernel.org/show_bug.cgi?id=13577 >> >> With this patch applied, only the first ACPI video bus device >> under a PCI device node is bind to ACPI video driver. >> >> The questions is that, we never notice this kind of devices before, >> thus I'm not sure this won't break any laptops. > > Laptops often have more than one video bus device, and you _have_ to choose > the one that is _active_ (which might not be the first one you find). This > is done on laptops that can have either discrete or in-chipset graphics, for > example. I have seen it in several thinkpad models. The video subsystem already picks only graphics devices that actually exist in the PCI bus. The issue here is that there are two ACPI "subdevices" under one address reference to a PCI device (which exists), and both have backlight functionality. I am not aware of any standard method to distinguish between such devices - I'd say it's probably yet another DSDT bug.
On Thu, 2009-07-16 at 05:59 +0800, Hector Martin wrote: > Henrique de Moraes Holschuh wrote: > > On Wed, 15 Jul 2009, Zhang Rui wrote: > >> Only one ACPI video bus device for a VGA controller. > >> > >> Some buggy BIOS exports multiple ACPI video bus devices for the same > >> VGA controller, and multiple backlight control methods as well. > >> This messes up the ACPI video backlight control. > >> http://bugzilla.kernel.org/show_bug.cgi?id=13577 > >> > >> With this patch applied, only the first ACPI video bus device > >> under a PCI device node is bind to ACPI video driver. > >> > >> The questions is that, we never notice this kind of devices before, > >> thus I'm not sure this won't break any laptops. > > > > Laptops often have more than one video bus device, and you _have_ to choose > > the one that is _active_ (which might not be the first one you find). This > > is done on laptops that can have either discrete or in-chipset graphics, for > > example. I have seen it in several thinkpad models. > > The video subsystem already picks only graphics devices that actually > exist in the PCI bus. The issue here is that there are two ACPI > "subdevices" under one address reference to a PCI device (which exists), > and both have backlight functionality. right. For example, here is a laptop with both an integrated graphics (\_SB.PCI0.GFX0) and an external graphics (\_SB.PCI0.NVDI) we can find which one is active, NVDI or GFX0, and ignore the inactive one. But the bug that I want to fix in this patch is that, there are two ACPI video bus devices \_SB.PCI0.GFX0.VID1 \_SB.PCI0.GFX0.VID2 both are active because PCI device GFX0 exists, and ACPI tries to bind all these two devices to ACPI video driver. it's meaningless to have two ACPI video bus device for one VGA controller, right? and it messes up the backlight controller because notifications are sent to both VID1 and VID2 when brightness hotkey is pressed. thanks, rui -- 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
On Thu, 16 Jul 2009, Zhang Rui wrote: > > > Laptops often have more than one video bus device, and you _have_ to choose > > > the one that is _active_ (which might not be the first one you find). This > > > is done on laptops that can have either discrete or in-chipset graphics, for > > > example. I have seen it in several thinkpad models. > > > > The video subsystem already picks only graphics devices that actually > > exist in the PCI bus. The issue here is that there are two ACPI > > "subdevices" under one address reference to a PCI device (which exists), > > and both have backlight functionality. > > right. > For example, > here is a laptop with both an integrated graphics (\_SB.PCI0.GFX0) and > an external graphics (\_SB.PCI0.NVDI) > we can find which one is active, NVDI or GFX0, and ignore the inactive > one. Well, those will likely change at runtime if we do our job properly to support dual-GPU laptops that can change which one is active at runtime... but hotplug support should be able to deal with it, but I don't know if the ACPI video device does runtime hotplug/unplug and runtime bind/unbind properly. Does it? However, some laptops (like my T43) do have two video devices, one of which will never be enabled (because it is there for a separate mainboard which has a different GPU). This is correctly handled currently. > But the bug that I want to fix in this patch is that, > there are two ACPI video bus devices > \_SB.PCI0.GFX0.VID1 > ???\_SB.PCI0.GFX0.VID2 > both are active because PCI device GFX0 exists, and ACPI tries to bind > all these two devices to ACPI video driver. > it's meaningless to have two ACPI video bus device for one VGA > controller, right? Unless your ACPI tables control each CRTC of the device separately... most GPUs have at least two CRTCs nowadats, and that might mean two backlight controllers as well, btw. But this doesn't look like what happens on the platform with the bug by the descriptions in this thread, and I know of no device that would try to do something as crazy as what I described (if it is allowed at all by the ACPI spec, which I didn't check)... > and it messes up the backlight controller because notifications are sent > to both VID1 and VID2 when brightness hotkey is pressed. If you had *two* backlight controllers, that should just mean you are to step them up/down together, nothing else... However, if they handle the same brightness hardware, yes, it _will_ cause problems (double stepping, for example).
Henrique de Moraes Holschuh wrote: >> But the bug that I want to fix in this patch is that, >> there are two ACPI video bus devices >> \_SB.PCI0.GFX0.VID1 >> ???\_SB.PCI0.GFX0.VID2 >> both are active because PCI device GFX0 exists, and ACPI tries to bind >> all these two devices to ACPI video driver. >> it's meaningless to have two ACPI video bus device for one VGA >> controller, right? > > Unless your ACPI tables control each CRTC of the device separately... most > GPUs have at least two CRTCs nowadats, and that might mean two backlight > controllers as well, btw. But this doesn't look like what happens on the > platform with the bug by the descriptions in this thread, and I know of no > device that would try to do something as crazy as what I described (if it is > allowed at all by the ACPI spec, which I didn't check)... VID1 and VID2 each are "video bus devices" and have children for each output device. Both have an LCD panel output device and both panels have backlight controls. Both backlight controls control the same hardware. The ACPI code in Linux already has support for this extra level of nesting (if _ADR is zero it checks to see if the parent represents the actual PCI device). The issue is that there are *two* children under the same PCI device. As far as Linux support goes, they are basically equivalent. > If you had *two* backlight controllers, that should just mean you are to > step them up/down together, nothing else... However, if they handle the > same brightness hardware, yes, it _will_ cause problems (double stepping, > for example). Right, they do. This is why one needs to be disabled. I don't think the patch should cause any issues because it ought to only disable extra devices that fall under the same parent PCI device. I don't think getting two video bus devices for one PCI device is sane in any instance. Here's the actual example from my DSDT: ------------------------------------------------------------- Device (PEGP) { Name (_ADR, 0x00010000) Device (EVGA) { Name (_ADR, Zero) [...] Device (CRT) { [...] } Device (LCD) { [...] [backlight support here] } Device (TV) { [...] } } Device (NVGA) { Name (_ADR, Zero) [...] Device (CRT0) { [...] } Device (LCD0) { [...] [backlight support here] } Device (HDV0) { [...] } [some extra methods] } [...] } Device (OVGA) { Name (_ADR, 0x00020000) [...] Device (CRT1) { [...] } Device (DTV1) { [...] } Device (LCD) { [...] [backlight support here] } Device (DD01) { [...] } Device (DD02) { [...] } Device (DD03) { [...] } Device (DD04) { [...] } Device (DD05) { [...] } [...] } ------------------------------------------------------------- As you can see, I have three video bus devices (each with subdevices for the multiple output types): \_SB.PCI0.PEGP.EVGA \_SB.PCI0.PEGP.NVGA \_SB.PCI0.OVGA OVGA corresponds to a nonexistent onboard video device (for other motherboards) and is ignored. EVGA and NVGA don't have addresses themselves and both correspond to the "PEGP" PCI device which is the MXM slot on the back of the laptop where the graphics device lives. All three video devices have the same backlight control hardware because the EC does that, not the video devices. The problem here is that video.ko grabs both EVGA and NVGA and makes two backlight control devices which causes issues when they control the same hardware. The patch is supposed to only grab one of the devices under a single PCI device (in this case it grabs EVGA). This works fine. In my case it seems that NVGA is the "correct" device (I have an NVidia card, and NVGA has some MXM-related information too which is referenced from some other WMI methods elsewhere), but it doesn't matter for video.ko since the only usable support I can get is backlight adjustment which works the same way in NVGA and EVGA.
applied to bugzilla-13577-video-TEST branch in acpi-test thanks, Len Brown, Intel Open Source Technology Center -- 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
Index: linux-2.6/drivers/acpi/video.c =================================================================== --- linux-2.6.orig/drivers/acpi/video.c +++ linux-2.6/drivers/acpi/video.c @@ -2195,11 +2195,43 @@ static int acpi_video_resume(struct acpi return AE_OK; } +static acpi_status +acpi_video_bus_match(acpi_handle handle, u32 level, void *context, + void **return_value) +{ + struct acpi_device *device = context; + struct acpi_device *sibling; + int result; + + if (handle == device->handle) + return AE_CTRL_TERMINATE; + + result = acpi_bus_get_device(handle, &sibling); + if (result) + return AE_OK; + + /* only one ACPI bus video device under a PCI device */ + if (!strcmp(acpi_device_name(sibling), ACPI_VIDEO_BUS_NAME)) + return AE_ALREADY_EXISTS; + + return AE_OK; +} + static int acpi_video_bus_add(struct acpi_device *device) { struct acpi_video_bus *video; struct input_dev *input; int error; + acpi_status status; + + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, + device->parent->handle, 1, + acpi_video_bus_match, device, NULL); + if (status == AE_ALREADY_EXISTS) { + printk(KERN_WARNING PREFIX, "Duplicate ACPI video bus " + "devices for the same VGA controller\n"); + return -ENODEV; + } video = kzalloc(sizeof(struct acpi_video_bus), GFP_KERNEL); if (!video)