[PATCH -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[PATCH -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[Zhang Rui]

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.

I suggest we put this patch in ACPI test tree first.

Tested-by: Hector Martin <hector@marcansoft.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

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)

Re: [PATCH -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[Henrique de Moraes Holschuh]

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

-- 
  "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 -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[Hector Martin]

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.

-- 
Hector Martin (hector@marcansoft.com)
Public Key: http://www.marcansoft.com/marcan.asc

Re: [PATCH -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[Zhang Rui]

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

Re: [PATCH -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[Henrique de Moraes Holschuh]

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

-- 
  "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 -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[Hector Martin]

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.

-- 
Hector Martin (hector@marcansoft.com)
Public Key: http://www.marcansoft.com/marcan.asc

Re: [PATCH -mm] ACPI video: only one ACPI bus video device is allowed for one VGA controller

[Len Brown]

applied to bugzilla-13577-video-TEST branch in acpi-test

thanks,
Len Brown, Intel Open Source Technology Center