Bug 9277

Summary: ACPI video driver should validate brightness level before setting it via _BCM
Product: ACPI Reporter: Danny Baumann (dannybaumann)
Component: Power-VideoAssignee: Zhang Rui (rui.zhang)
Status: CLOSED PATCH_ALREADY_AVAILABLE    
Severity: normal CC: acpi-bugzilla, belegdol, moneta.mace, thoemy
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.23 Subsystem:
Regression: --- Bisected commit-id:
Attachments: Implement brightness validation before calling _BCM
Don't set current brightness variable from acpi_video_device_write_brightness
video brightness control fixup
set the proper brightness levels
patch vs 2.6.24

Description Danny Baumann 2007-11-02 06:15:23 UTC
Most recent kernel where this bug did not occur: none
Distribution: Fedora 8
Problem Description:
The function acpi_video_device_set_level should validate the brightness level passed to it using the brightness levels obtained from _BCL. This is required in the ACPI spec, section B.6.3.
Comment 1 Danny Baumann 2007-11-02 06:17:37 UTC
Created attachment 13372 [details]
Implement brightness validation before calling _BCM

This patch implements the required validation. It also removes this validation from acpi_video_device_write_brightness as the validation would be duplicated otherwise.
Comment 2 Danny Baumann 2007-11-02 06:19:02 UTC
Created attachment 13373 [details]
Don't set current brightness variable from acpi_video_device_write_brightness

We don't need to set brightness->curr from acpi_video_device_write_brightness. acpi_video_device_lcd_set_level already does that.
Comment 3 Danny Baumann 2007-12-18 02:24:49 UTC
Any updates on the status of this problem and patch? Anything I can do/change to get this bit included?
Comment 4 Zhang Rui 2007-12-18 18:22:00 UTC
It seems that Len didn't merge this patch.
Comment 5 Zhang Rui 2007-12-18 18:25:11 UTC
Created attachment 14114 [details]
video brightness control fixup

Hi, Danny,
Any comment on this patch? :)
Comment 6 Julian Sikorski 2007-12-20 13:34:36 UTC
Hi.

Some time ago, when Fn keys on my Satellite A100 used to work, I did monitor their behaviour with omnibook module [1]. It can only read brightness value for this model, not set it. It turned out then, that every keypress changed the brightness by two values (7 -> 5 -> 3 -> 1 -> 0). There are eight brightness levels supported by this laptop. Now, it is possible to set the desired brightness level by setting a value in /proc/omnibook/lcd, and then pressing fn-f6 or fn-f7 once. Reading the value always returns 0 or 7.
Now the behaviour is screwed up, but at least the buttons work.

[1] http://omnibook.sf.net
Comment 7 Zhang Rui 2007-12-20 17:05:39 UTC
I'm not clear about what the omnibook is, but it's apparently not an acpi issue, right?
Sorry that I can't help you on this problem. Please open another bug and assign it to the proper category. :)

Danny, will you please review the patch in comment #5?
Comment 8 Danny Baumann 2007-12-21 00:05:20 UTC
(In reply to comment #7)
> I'm not clear about what the omnibook is, but it's apparently not an acpi
> issue, right?

Omnibook is a custom ACPI driver for Toshiba and HP notebooks made by Compal.

> Sorry that I can't help you on this problem. Please open another bug and
> assign
> it to the proper category. :)

Agreed, the issue in comment #6 looks like the "duplicate video device" issue, so I would also propose opening a new bug with an acpidump attached.

> Danny, will you please review the patch in comment #5?

Will do ASAP - the patch itself looks fine to me, I just need to find some time to build a kernel and test.
Comment 9 Julian Sikorski 2007-12-21 00:24:07 UTC
Sorry for causing confusion, I'll open a new bug for duplicate video device. I just thought that these issues are related.
Comment 10 Danny Baumann 2007-12-21 08:30:44 UTC
(In reply to comment #5)
> Created an attachment (id=14114) [details]
> video brightness control fixup
> 
> Hi, Danny,
> Any comment on this patch? :)
It seems to work fine. Just two comments:
- The value "2" in the validation loop deserves a small comment IMO - it's not intuitive that the first two values in the list are default values ;)
- The brightness list returned by _BCL is not guaranteed to be sorted by values (at least I couldn't find an order requirement in the spec), which your code assumes. I don't know how relevant this is, but we should keep this in mind.
Comment 11 Zhang Rui 2007-12-26 19:56:50 UTC
Created attachment 14196 [details]
set the proper brightness levels
Comment 12 Zhang Rui 2008-01-02 00:27:03 UTC
Len, is this patch in acpi test tree now? :)
Comment 13 Julian Sikorski 2008-01-04 03:06:09 UTC
I can confirm that this patch fixes the flicker problem with gnome-power-manager. Remaining issues are most likely caused by duplicate video device issue, described in bug #9614.
Comment 14 Len Brown 2008-01-08 21:21:12 UTC
patch in comment #11 applied to acpi test
Comment 15 Len Brown 2008-02-01 19:51:14 UTC
Created attachment 14680 [details]
patch vs 2.6.24

patch in comment #11 dropped from acpi-test.

This patch from Matthew Garret applied instead.
Rather than (fake) percentages, the generic Linux API
wants a series of settings from 0 to max_brightness
all of which do something useful.
Comment 16 Julian Sikorski 2008-02-02 08:00:13 UTC
This one works even better. Thanks.
Comment 17 Len Brown 2008-02-10 14:33:28 UTC
thanks for testing.

the patch in comment #15 shipped in Linux-2.6.24-rc22.

closed.
Comment 18 Julian Sikorski 2008-02-10 14:37:51 UTC
2.6.24-rc22?
Comment 19 Len Brown 2008-02-10 14:56:30 UTC
ups, i meant 2.6.24-git22
that is the release 22 days after 2.6.24, and presumably
just before 2.6.25-rc1.
Comment 20 Julian Sikorski 2008-02-10 14:58:55 UTC
IC, thanks for clarification.