ACPI / PM: Traverse video_device_list for backlight restoration
PATCH 1/1 ACPI / PM: Traverse video_device_list for backlight restoration
.bind_info in active_list[] is NULL by construction - and used to determine the acpi_video_device pointer during backlight restoration during resume.
Using the list instead fixes backlight restoration for systems where the BIOS doesn't do this by itself already.
--- drivers/acpi/video.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/acpi/video.c +3 -3
@@ -2346,7 +2346,6 @@ static int acpi_video_resume(struct notifier_block *nb, {struct acpi_video_bus *video;struct acpi_video_device *video_device; -int i;switch (val) {case PM_HIBERNATION_PREPARE :
@@ -2359,11 +2358,12 @@ static int acpi_video_resume(struct notifier_block *nb,dev_info (&video->device->dev,"Restoring backlight state\n" ); -for (i =0 ; i < video->attached_count; i++) { - video_device = video->attached_array[i].bind_info; +mutex_lock (&video->device_list_lock); +list_for_each_entry (video_device, &video->video_device_list, entry) {if (video_device && video_device->backlight)acpi_video_set_brightness (video_device->backlight); } +mutex_unlock (&video->device_list_lock);return NOTIFY_OK ; }
Signature · 1 line
1.6.0.2
A bit of background about this patch:
acpi_video_bus_get_devices() explicitly calls acpi_video_device_enumerate() before acpi_video_bus_get_one_device(), which does acpi_video_device_bind().
In _enumerate(), active_list[] is created, with .bind_info explicitly set to NULL, while the related .bind_info entry in video_device_list is set to the acpi_video_device ptr in _bind().
The .bind_info is later used in acpi_video_resume() to re-set the backlight
- but it's only evaluated on the active_list[], on which all .bind_info are NULL by construction. This results in backlight not being restored, if the BIOS doesn't do this by itself already.
The patch resolves this by replacing the active_list[] traversal by a video_device_list traversal, but due to the nature of the issue I'm unsure whether the original issue isn't of more principal quality. I don't understand why the list is (partially) transformed into an array in the first place, especially as both the array *and* the list are used in the code...
Thanks
Matthias
Signature · 3 lines
Matthias Hopf <[email protected]> __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ [email protected] Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de
A bit of background about this patch:
acpi_video_bus_get_devices() explicitly calls acpi_video_device_enumerate() before acpi_video_bus_get_one_device(), which does acpi_video_device_bind().
In _enumerate(), active_list[] is created, with .bind_info explicitly set to NULL, while the related .bind_info entry in video_device_list is set to the acpi_video_device ptr in _bind().
The .bind_info is later used in acpi_video_resume() to re-set the backlight
- but it's only evaluated on the active_list[], on which all .bind_info are NULL by construction. This results in backlight not being restored, if the BIOS doesn't do this by itself already.
The patch resolves this by replacing the active_list[] traversal by a video_device_list traversal, but due to the nature of the issue I'm unsure whether the original issue isn't of more principal quality. I don't understand why the list is (partially) transformed into an array in the first place, especially as both the array *and* the list are used in the code...
Thanks
Matthias
Signature · 3 lines
Matthias Hopf <[email protected]> __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ [email protected] Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de
Quoted 6 lines · Matthias Hopf
A bit of background about this patch:
acpi_video_bus_get_devices()explicitly callsacpi_video_device_enumerate()beforeacpi_video_bus_get_one_device(), which doesacpi_video_device_bind().
Quoted 7 lines
In
_enumerate(), active_list[] is created, with .bind_info explicitly set to NULL, while the related .bind_info entry invideo_device_listis set to theacpi_video_deviceptr in_bind().The .bind_info is later used in
acpi_video_resume()to re-set the backlight
- but it's only evaluated on the active_list[], on which all .bind_info are NULL by construction.
why? .bind_info in active_list is initialized in acpi_video_device_bind.
Quoted 7 lines
This results in backlight not being restored, if the
BIOS doesn't do this by itself already.The patch resolves this by replacing the active_list[] traversal by a
video_device_listtraversal,
Quoted 8 lines
but due to the nature of the issue I'm unsure
whether the original issue isn't of more principal quality.I don't understand
why the list is (partially) transformed into an array in the first place, especially as both the array *and* the list are used in the code...
no, the active_list is an array in fact, which equals acpi_video_bus->attached_array. Maybe we need to change it to a more proper name.
I'm still wondering why the patch works for you. could you please attach the acpidump output of this laptop?
please rebuild your kernel with CONFIG_ACPI_DEBUG=y, and reboot with kernel parameter "acpi.debug_layer=0x10 acpi.debug_level=0x400". and attach the dmesg output after resume, both w/ and w/o the patch you attached.
thanks,
rui
Quoted 5 lines · Zhang Rui
Quoted 3 lines · Matthias Hopf
The .bind_info is later used in
acpi_video_resume()to re-set the backlight
- but it's only evaluated on the active_list[], on which all .bind_info are NULL by construction.why? .bind_info in
active_listis initialized inacpi_video_device_bind.
Hm. Right. acpi_video_bus_get_one_device() is called after _enumerate(). Dunno why I didn't read this correctly.
Quoted 7 lines
Quoted 4 lines
I don't understand
why the list is (partially) transformed into an array in the first place, especially as both the array *and* the list are used in the code...no, the
active_listis an array in fact, which equalsacpi_video_bus->attached_array. Maybe we need to change it to a more proper name.
:-)
Quoted 1 line
I'm still wondering why the patch works for you.
Same to me.
Quoted 1 line
could you please attach the acpidump output of this laptop?
Will do.
Quoted 4 lines
please rebuild your kernel with
CONFIG_ACPI_DEBUG=y, and reboot with kernel parameter "acpi.debug_layer=0x10 acpi.debug_level=0x400". and attach the dmesg output after resume, both w/ and w/o the patch you attached.
The most intriguing thing here is that the system starts writing to /var/log/messages continuously, thus dmesg output is sort-of worthless. This might be a reasonable side effect, it might be an indication of the original issue.
I'll try to compile something for you.
Thanks
Matthias
Signature · 3 lines
Matthias Hopf <[email protected]> __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ [email protected] Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de
Quoted 5 lines · Zhang Rui
Quoted 3 lines · Matthias Hopf
The .bind_info is later used in
acpi_video_resume()to re-set the backlight
- but it's only evaluated on the active_list[], on which all .bind_info are NULL by construction.why? .bind_info in
active_listis initialized inacpi_video_device_bind.
Hm. Right. acpi_video_bus_get_one_device() is called after _enumerate(). Dunno why I didn't read this correctly.
Quoted 7 lines
Quoted 4 lines
I don't understand
why the list is (partially) transformed into an array in the first place, especially as both the array *and* the list are used in the code...no, the
active_listis an array in fact, which equalsacpi_video_bus->attached_array. Maybe we need to change it to a more proper name.
:-)
Quoted 1 line
I'm still wondering why the patch works for you.
Same to me.
Quoted 1 line
could you please attach the acpidump output of this laptop?
Will do.
Quoted 4 lines
please rebuild your kernel with
CONFIG_ACPI_DEBUG=y, and reboot with kernel parameter "acpi.debug_layer=0x10 acpi.debug_level=0x400". and attach the dmesg output after resume, both w/ and w/o the patch you attached.
The most intriguing thing here is that the system starts writing to /var/log/messages continuously, thus dmesg output is sort-of worthless. This might be a reasonable side effect, it might be an indication of the original issue.
I'll try to compile something for you.
Thanks
Matthias
Signature · 3 lines
Matthias Hopf <[email protected]> __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ [email protected] Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de
Quoted 6 lines · Matthias Hopf
A bit of background about this patch:
acpi_video_bus_get_devices()explicitly callsacpi_video_device_enumerate()beforeacpi_video_bus_get_one_device(), which doesacpi_video_device_bind().
Quoted 7 lines
In
_enumerate(), active_list[] is created, with .bind_info explicitly set to NULL, while the related .bind_info entry invideo_device_listis set to theacpi_video_deviceptr in_bind().The .bind_info is later used in
acpi_video_resume()to re-set the backlight
- but it's only evaluated on the active_list[], on which all .bind_info are NULL by construction.
why? .bind_info in active_list is initialized in acpi_video_device_bind.
Quoted 7 lines
This results in backlight not being restored, if the
BIOS doesn't do this by itself already.The patch resolves this by replacing the active_list[] traversal by a
video_device_listtraversal,
Quoted 8 lines
but due to the nature of the issue I'm unsure
whether the original issue isn't of more principal quality.I don't understand
why the list is (partially) transformed into an array in the first place, especially as both the array *and* the list are used in the code...
no, the active_list is an array in fact, which equals acpi_video_bus->attached_array. Maybe we need to change it to a more proper name.
I'm still wondering why the patch works for you. could you please attach the acpidump output of this laptop?
please rebuild your kernel with CONFIG_ACPI_DEBUG=y, and reboot with kernel parameter "acpi.debug_layer=0x10 acpi.debug_level=0x400". and attach the dmesg output after resume, both w/ and w/o the patch you attached.
thanks,
rui