On Sun, Mar 31, 2013 at 09:04:21AM +0200, Danny Baumann wrote:Hi,
Artem Savkov <artem.savkov@xxxxxxxxx> schrieb:
On Sun, Mar 31, 2013 at 08:15:14AM +0200, Danny Baumann wrote:Artem Savkov <artem.savkov@xxxxxxxxx> schrieb:returnedacpi_video_device_lcd_get_level_current() called
acpi_video_bqc_value_to_level()
with "*level" as a second argument, resulting in level being
parameter into the evaluation of _BQC, *level contains the AML returnedbased onI don't think this change is correct. As level was passed as
initial input, not current brightness, breaking backlight controls.
brightness level afterwards, so it's correct to use it as an input to
acpi_video_bqc_value_to_level(). Actually, the whole point of
acpi_video_device_lcd_get_level_current() is to update
device->brightness->curr, so it doesn't make sense to me to use it in
that function.
What's the exact problem this patch tries to solve?
I'm running a -next kernel on my laptop and couple of days ago keyboard
backlight controls stopped working: only 2 lower brightness levels.
I've
debugged it a bit and found out that acpi_video_switch_brightness()
calls
acpi_video_device_lcd_get_level_current() with level uninitialized and
always gets lowest posible value.
The point is: after the acpi_evaluate_object call, *level must contain the current brightness level, otherwise your BIOS is broken (this happens: e.g. my laptop always returns 100 from _BQC in Windows 8 mode). You can verify this easily by initializing level_current to some invalid value and checking it again after the call to _get_level_current. I'd be pretty surprised if the value didn't change. Also, if you look at [1], you'll see that the code operated on *level before as well.
This problem may have been obscured by the fact that the brightness device wasn't initialized properly before my patches went in. Does acpi_video_switch_brightness actually do anything for you when reverting the 3 newest commits of video.c?
Yes, you are right. Both my patch and the version before your patches
result in initial acpi_video_device_lcd_get_level_current call ending
with invalid level being returned and bqc disabled, so they both just
use *level = brightness->curr after that.