Bug 9614

Summary: Duplicate video device for Toshiba Satellite A100-847
Product: ACPI Reporter: Julian Sikorski (belegdol)
Component: Power-VideoAssignee: Thomas Renninger (trenn)
Status: CLOSED PATCH_ALREADY_AVAILABLE    
Severity: high CC: acpi-bugzilla, bunk, corsac, dannybaumann, jbarnes, marcus, pittipatti, sam, trenn
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.23.9-85.fc8 Subsystem:
Regression: --- Bisected commit-id:
Attachments: lspci output
dmesg output
ACPI tables dump
dmesg output with ACPI debugging enabled
patch-ignore-acpi-video-devices
Cleanup: Initialize brightness and display switching in separate functions
ACPI: video: Ignore devices that aren't present in hardware
Check for backlight support via ACPI video.ko otherwise use vendor ACPI drivers
Oops, forgot to add this file. Now it should work
Clean patch that replaces the last two.

Description Julian Sikorski 2007-12-21 01:24:38 UTC
Most recent kernel where this bug did not occur: none
Distribution: Fedora
Problem Description:
There is a duplicate video device in Toshiba Satellite A100-847 laptop:

[jsikorski@snowball ~]$ lshal | grep laptop_panel
  info.capabilities = {'laptop_panel'} (string list)
  info.category = 'laptop_panel'  (string)
  laptop_panel.access_method = 'general'  (string)
  laptop_panel.num_levels = 101  (0x65)  (int)
  info.capabilities = {'laptop_panel'} (string list)
  info.category = 'laptop_panel'  (string)
  laptop_panel.access_method = 'general'  (string)
  laptop_panel.num_levels = 101  (0x65)  (int)
[jsikorski@snowball ~]$ 

This probably causes the problem that every Fn-F6 or Fn-F7 keypress the brightness is changed by two values, as monitored by omnibook [1]. Now the behaviour is even more broken, most probably due to bug #9277. Every keypress gives two ACPI events:

10:22:54.773: computer_logicaldev_input_0 condition ButtonPressed = brightness-down
10:22:54.773: computer_logicaldev_input_4 condition ButtonPressed = brightness-down

[1] http://omnibook.sourceforge.net
Comment 1 Julian Sikorski 2007-12-21 01:25:06 UTC
Also, there should only be 8 brightness levels.
Comment 2 Fu Michael 2007-12-24 16:59:37 UTC
would you please attach:

1)lspci -vvxxx
2)acpidump
3)dmesg

thanks.
Comment 3 Julian Sikorski 2007-12-25 00:51:22 UTC
Created attachment 14177 [details]
lspci output
Comment 4 Julian Sikorski 2007-12-25 00:51:58 UTC
Created attachment 14178 [details]
dmesg output
Comment 5 Julian Sikorski 2007-12-25 00:54:09 UTC
acpidump will have to wait as this package is not present in Fedora Package Collection.
Comment 6 Fu Michael 2007-12-26 00:50:26 UTC
we still need the acpidump info to confirm. thanks.
Comment 7 Julian Sikorski 2007-12-26 03:38:34 UTC
Created attachment 14188 [details]
ACPI tables dump

I obtained this one using Linux Firmware Kit R3. Please let me know if it is the file you required.
Comment 8 Zhang Rui 2008-01-02 01:16:16 UTC
Hmm, please set CONFIG_ACPI_DEBUG and
kill acpid
"cat /proc/acpi/event"
"echo 0x8800001f > /sys/module/acpi/parameters/debug_level"
"echo 0x04 > /sys/module/acpi/parameters/debug_layer"
and press the hotkey Fn-F6 and Fn-F7 one time for each
and attach the dmesg output and the result of /proc/acpi/event.
Comment 9 Julian Sikorski 2008-01-02 01:57:06 UTC
Created attachment 14260 [details]
dmesg output with ACPI debugging enabled

[root@snowball ~]# cat /proc/acpi/event
video LCD 00000087 00000000
video LCD 00000087 00000000
video LCD 00000086 00000000
video LCD 00000086 00000000

I also had to stop haldaemon, since it was blocking the access to /proc/acpi/event. The logs were obtained using 2.6.23.9-85.fc8debug kernel.
Comment 10 Julian Sikorski 2008-01-02 02:08:18 UTC
Also, looks like stopping haldaemon resolved the gnome-power-manager issues, mentioned in bug #9262 and #9277.
Comment 11 Zhang Rui 2008-01-02 17:40:00 UTC
Method (_Q60, 0, NotSerialized)
{
...
If (LEqual (Local0, 0x41))
   {
   Notify (^^^GFX0.LCD, 0x86)
   Notify (^^^PEGP.VGA.LCD, 0x86)
   }
...
}
and 
Method (_Q64, 0, NotSerialized)
     {
     Notify (^^^GFX0.LCD, 0x86)
     Notify (^^^PEGP.VGA.LCD, 0x86)
     }
Yes, by checking the DSDT table you attached, BIOS sends two notifications for one key press each time.
ACPI can never know which video bus actually works. It just executes the BIOS code and exports the notifications to user space. And unfortunately, the two LCD devices have the same name in this laptop, thus user space changes the brightness level twice in a row...

We met the duplicate video device problems before, but there is still not a fix for this. :(.
For your box, you probably need to fix it in userspace for now. ie, modify the script to change one brightness level per two acpi events.
Comment 12 Julian Sikorski 2008-01-02 21:15:06 UTC
Thank you for the feedback. If such problem is common, maybe it would be worth to create a hal quirk doing that? There is one (laptop_panel.brightness_in_hardware), but I am not sure if it is supposed to fix the issue we have here. I'll try it out.
Comment 13 Zhang Rui 2008-01-03 22:49:58 UTC
I have a quick look at laptop_panel.brightness_in_hardware, I'm afraid it can't fix your problem at hand.

I think we really need to get a proposal on how to fix it in kernel.
Duplicate video bus device shown in ACPI namespace, one stands for the integraded VGA controller, while another stands for an external graphics card.
There was a patch sent by thomas for fixing this problem, you may have a try.
http://marc.info/?l=linux-acpi&m=119703212318089&w=2
Comment 14 Julian Sikorski 2008-01-04 06:10:51 UTC
The patch from comment #13 fixes the issue for me. Without haldaemon and g-p-m running, I can switch through 8 values of brightness, as under windows. There is only one device visible as well.
If haldaemon and g-p-m are running, one issue remains. There are nine steps down (the last one does not really do anything) and eight steps up. I suspect that's because the percentage values do not overlap with eight available levels very well. I'll try laptop_panel.brightness_in_hardware quirk and report back.
Comment 15 Julian Sikorski 2008-01-04 06:21:27 UTC
The quirk does not change anything here. Looks like a bug in gnome-power-manager then - the two lowest values are reported by /proc/acpi/video/VGA/LCD/brightness as 10.
Comment 16 Thomas Renninger 2008-01-07 05:31:56 UTC
Rui, can you point Len's attention to this one, pls.
I posted this on acpi list twice. Matthew Garrett had some concerns that some graphics devices could get rejected wrongly, but this has to be proven by a real machine/DSDT first (and can then get fixed up more fine grained if necessary).
IMO my patch should be added, I thought it already had.
Comment 17 Zhang Rui 2008-01-21 23:28:36 UTC
Created attachment 14529 [details]
patch-ignore-acpi-video-devices

can you please try this patch and make sure it works for you?
Comment 18 Danny Baumann 2008-01-22 02:01:20 UTC
(In reply to comment #17)
> Created an attachment (id=14529) [details]
> patch-ignore-acpi-video-devices
> 
> can you please try this patch and make sure it works for you?

Works fine for me. With it, I only have one video bus left. Consequently, brightness control works as it should. This is on a Toshiba Satellite Pro A100-080.
Comment 19 Julian Sikorski 2008-01-22 02:07:34 UTC
Works for me as well. There is only one video device left (/proc/acpi/video/VGA, as opposed to /proc/acpi/video/VGA and /proc/acpi/video/GFX-0 before), and along with the patch from bug #9277, brightness controls work like charm.
Comment 20 Julian Sikorski 2008-01-22 02:10:22 UTC
FYI, this laptop was sold as “Designed for Windows XP, Windows Vista Capable”. I did update the BIOS several times since I bought it, though.
Comment 21 Len Brown 2008-02-01 23:16:18 UTC
applied patch in comment #17 to acpi test branch
Comment 22 Len Brown 2008-02-10 14:43:02 UTC
a cleaned up version of Matthew's patch in comment #17
shipped in linux-2.6.24-rc22
closed
Comment 23 Len Brown 2008-03-18 01:16:20 UTC
looks like i messed up.

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

indeed went upstream between 2.6.24 and 2.6.25-rc1 --
but it shouldn't have because it wasn't the final patch.

A later version of that patch also went upstream 
between 2.6.24 and 2.6.25-rc1

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

The two patches have the same function, but the text doesn't collide,
so I didn't notice the merge error.
Comment 24 Adrian Bunk 2008-03-18 10:23:00 UTC
commit 0119509c4fbc9adcef1472817fda295334612976 was reverted in Linus' tree
Comment 25 Len Brown 2008-03-25 20:35:22 UTC
Due to regression bug 9995, these patches were reverted
between 2.6.25-rc6 and 2.6.25-rc6-git8 in the following commits:

0d6752c9fa51d24c86b57c76ec5b2926a716b23
Revert "ACPI: video: Ignore ACPI video devices that aren't present in hardware"
77321e624b64f1e5985a20f3cd16b94c96d0dbb4
Revert "ACPI: video: Ignore devices that aren't present in hardware"

so this sighting is REOPENED.
Comment 26 Yves-Alexis Perez 2008-05-04 09:45:52 UTC
I have a thinkpad T61 with the same kind of bugs.

Using 2.6.25.1, I have two acpi_video* in /sys/class/backlight.

Brightness keys manage to change both acpi_videoX/actual_brightness, but acpi_videoX/brightness doesn't change.

echo XX > acpi_video1/brightness changes acpi_video1/brightness and acpi_video1/actual_brightness _and_ the effective LCD brightness

echo XX > acpi_video0/brightness changes acpi_video0/brightness and acpi_video0/actual_brightness, but LCD brightness doesn't change.

I'm on a T61 8897 CTO with integrated graphics, with bios 2.17 (the latest atm). If you want dmesg, lspci, acpidump stuff, please ask.

Cheers,
Comment 27 Yves-Alexis Perez 2008-05-15 13:05:10 UTC
Hmhm, is there any news on this stuff?
Comment 28 Zhang Rui 2008-06-03 17:58:02 UTC
no. :(
we still don't have a solution for this problem
Comment 29 Thomas Renninger 2008-07-01 07:07:25 UTC
Created attachment 16673 [details]
Cleanup: Initialize brightness and display switching in separate functions

These three patches should fix the problem:
  - Make ThinkPads with Intel graphics card using the right device
  - Ignore devices not plugged in making Toshiba and other models work with
    video.ko
  - Fix possible double HW access when switching brightness through vendor
    specific drivers

Tested backlight switching on a ThinkPad T61 with discrete Nvidia and on a T61 with an internal Intel graphics card (without X!).

Please give them a try.
Comment 30 Thomas Renninger 2008-07-01 07:08:18 UTC
Created attachment 16674 [details]
ACPI: video: Ignore devices that aren't present in hardware
Comment 31 Thomas Renninger 2008-07-01 07:11:25 UTC
Created attachment 16675 [details]
Check for backlight support via ACPI video.ko otherwise use vendor ACPI drivers

All these are against Len's latest release git tree.

Some minor parts in thinkpad_acpi need cleanup if this should go in.
Henrique (and hopefully others) should have a look/review anyway.
Comment 32 Thomas Renninger 2008-07-01 09:07:35 UTC
Created attachment 16676 [details]
Oops, forgot to add this file. Now it should work

There still should be a compile error if video is not selected in .config, better enable it for testing
Comment 33 Thomas Renninger 2008-07-02 06:15:40 UTC
Created attachment 16694 [details]
Clean patch that replaces the last two.

We now have three patches:
  - Two more or less cleanups
  - One huge one that checks for video support in ACPI before any module gets
    loaded. Later (this is why so many drivers are affected) every vendor
    specific ACPI driver must check whether video.ko is going to do backlight
    switching or whether it can access the hardware.
Comment 34 Julian Sikorski 2008-07-06 02:08:08 UTC
I'll have a look at it soonish, just have to find some time to rebuild the kernel.
Comment 35 Julian Sikorski 2008-07-06 03:11:52 UTC
Looks like I won't be able to test it right now, since this patch does not apply against neither Fedora 9 nor Fedora rawhide kernels (which is probably not surprising) and I have no skills to build a git kernel.
Comment 36 Zhang Rui 2008-07-06 20:34:28 UTC
Hah, I have a T61 here which can reproduce the problem.
I've tested the patches and it works well.
But I still have one question,
some IGD bios supports both IGD and legacy mode to change backlight,
which can be switched by acpi_osi="!Windows 2006".
But this patch removes the sys backlight interface for all ACPI video device once "DRDY" is found.

So, IMO, IGD support should not be involved in the second patch.
The first patch only causes regressions on T61 laptops (works for others like Toshiba) and a dmicheck can workaround the problem for now, even before the IGD Opregion patches are upstream, right?
Comment 37 Thomas Renninger 2008-07-10 00:02:10 UTC
Zhang, thanks a lot for testing and looking through this!

> So, IMO, IGD support should not be involved in the second patch.
Yes, I will remove it in the next version.
I thought the IGD support takes longer, but maybe now all works out.
It would still be nice to be able to switch backlight without IGD (e.g. if parts of the BIOS are missing as Matthew stated), but this could be added on top if needed, I like to make this as easy as possible for now to get it upstream.

Be careful the patch has a bug which can lead to a kernel oops:
+       acpi_bus_get_device(handle, &acpi_dev);
in find_video() must be checked for it's return value.

I will send the next version on the list.
Comment 38 Thomas Renninger 2008-07-10 00:05:24 UTC
> The first patch only causes regressions on T61 laptops
Matthew stated that it could cause regressions on other laptops, e.g. Panasonic as well. But he did not explain the details, I also consider it unlikely that we have more machines showing regressions on the initial patch (do not let video.ko access not physically available ACPI devices).
Comment 39 Zhang Rui 2008-08-13 23:05:05 UTC
Re-assign this bug to Thomas.
And we can mark it as RESOLVED once Thomas has repost the patch set to linux-acpi.
Comment 40 Zhang Rui 2008-08-13 23:08:01 UTC
*** Bug 10448 has been marked as a duplicate of this bug. ***
Comment 41 Thomas Renninger 2008-08-19 06:15:10 UTC
The code is in Andi's test branch, but won't make it into .27.
AFAIK policy is to set the bug to resolved already -> doing so.
Comment 42 Patrick Scharrenberg 2008-09-03 01:50:13 UTC
Which branch are you talking about?
I cloned git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-acpi-2.6.git but can't find any related commits there.
I came here from bug 10448 which was marked as duplicate and describes the freezing of a Samsung q45 on loading video.ko.
Unfortunately the latest code from the git repository above still freezes my samsung and the latest patch found in this bug-report doesn't run cleanly against 2.6.25.
Thanks in advance.
Comment 43 Thomas Renninger 2008-09-03 02:41:47 UTC
Try:
> git-clone git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-acpi-2.6
> acpi_test
> git-branch -r
> git-branch --track test origin/test
> git-checkout test
> git-log  # and search for the last change from trenn
Comment 44 Patrick Scharrenberg 2008-09-04 01:29:08 UTC
With a kernel from that branch my Samsung q45 with intel graphics finally boots again without freezing while loading the video.ko-module.

Thanks!
Comment 45 Len Brown 2008-11-12 21:08:49 UTC
shipped in 2.6.28-rc4-git3

closed.

commit 22c13f9d8179f4c9caecfcb60a95214562b9addc
Author: Thomas Renninger <trenn@suse.de>
Date:   Fri Aug 1 17:37:54 2008 +0200

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