Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
On ven., 2013-09-27 at 12:20 -0300, Henrique de Moraes Holschuh wrote: Some testing on a *60 (T60,X60...) would also be best, I cannot test this on my T43. Anyway, the code itself looks fine, so: I can test on T61, would that help? Regards, -- Yves-Alexis signature.asc Description: This is a digitally signed message part ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
On ven., 2013-09-27 at 15:05 -0300, Henrique de Moraes Holschuh wrote: On Fri, 27 Sep 2013, Yves-Alexis Perez wrote: On ven., 2013-09-27 at 12:20 -0300, Henrique de Moraes Holschuh wrote: Some testing on a *60 (T60,X60...) would also be best, I cannot test this on my T43. Anyway, the code itself looks fine, so: I can test on T61, would that help? I think so. Ok, just tested on my T61 with Intel graphics. I've checked that on Linux 3.12 (6cac446bd37d9381815fe4c2b0e7b1fd1085000c), brightness keys work fine like they do in 3.2. Then I've applied the patchset. The brightness keys still work, I still have 16 levels. Regards, -- Yves-Alexis signature.asc Description: This is a digitally signed message part ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
On Thu, 26 Sep 2013, Aaron Lu wrote: I checked the git log for the commit to use tpacpi_acpi_handle_locate to locate video controller's ACPI handle, it's: commit 122f26726b5e16174bf8a707df14be1d93c49d62 Author: Henrique de Moraes Holschuh h...@hmh.eng.br Date: Mon Aug 9 23:48:18 2010 -0300 Yeah... So I checked out that commit and found that it shouldn't work either, since it has the same problem of the current code. The ACPI video controller device is given an id of ACPI_VIDEO_HID, but it's only known to Linux ACPI, not ACPICA, so the function provided by ACPICA acpi_get_devices will not work in this case, as that function will really check the control method of _HID under the handle, which does not exist no matter if Linux ACPI has added an id to its device structure or not. OTOH, the function provided by Linux ACPI acpi_device_hid will see the added id. In a word, the add of the HID will not affect the ASL namespace layout of the device node(thus, no _HID control method will be added to the device node). Erk. It went broken for a long time, and the users didn't notice(!)... commit ff413195e830541afeae469fc866ecd0319abd7e Author: Alex Hung alex.h...@canonical.com Date: Tue Apr 24 16:40:52 2012 +0800 thinkpad-acpi: fix issuing duplicated key events for brightness up/down The tp_features.bright_acpimode will not be set correctly for brightness control because ACPI_VIDEO_HID will not be located in ACPI. As a result, a duplicated key event will always be sent. acpi_video_backlight_support() is sufficient to detect standard ACPI brightness control. Signed-off-by: Alex Hung alex.h...@canonical.com Signed-off-by: Matthew Garrett m...@redhat.com Until that. And unfortunately I did not connect the dots at the time. Thanks for explaining the issue properly. -- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
On Tue, 24 Sep 2013, Aaron Lu wrote: The tpacpi_acpi_handle_locate function makes use of acpi_get_devices to locate handle for ACPI video by HID, the problem is, ACPI video node doesn't really have HID defined(i.e. no _HID control method is defined for video device), so.. that function would fail. This can be solved by enhancing the callback function for acpi_get_devices, where we can use acpi_device_hid function to check if the ACPI node corresponds to a video controller. In addition to that, the _BCL control method only exists under a video output device node, not a video controller device node. So to evaluate _BCL, we need the handle of a video output device node, which is child of the located video controller node from tpacpi_acpi_handle_locate. The two fix are necessary for some Thinkpad models to emit notification on backlight hotkey press as a result of evaluation of _BCL. Signed-off-by: Aaron Lu aaron...@intel.com Tested-by: Igor Gnatenko i.gnatenko.br...@gmail.com Some testing on a *60 (T60,X60...) would also be best, I cannot test this on my T43. Anyway, the code itself looks fine, so: Acked-by: Henrique de Moraes Holschuh h...@hmh.eng.br -- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
On Fri, 27 Sep 2013, Yves-Alexis Perez wrote: On ven., 2013-09-27 at 12:20 -0300, Henrique de Moraes Holschuh wrote: Some testing on a *60 (T60,X60...) would also be best, I cannot test this on my T43. Anyway, the code itself looks fine, so: I can test on T61, would that help? I think so. -- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
On Wed, Sep 25, 2013 at 04:58:39PM -0300, Henrique de Moraes Holschuh wrote: On Tue, 24 Sep 2013, Aaron Lu wrote: locate handle for ACPI video by HID, the problem is, ACPI video node doesn't really have HID defined(i.e. no _HID control method is defined ACPI video is supposed to attach a virtual HID node (ACPI_VIDEO_HID) to ACPI video devices so as to keep the non-trivial video device detection logic in just one place instead of reinventing the wheel in every driver (which is always a recipe for disaster). When did that break? I checked the git log for the commit to use tpacpi_acpi_handle_locate to locate video controller's ACPI handle, it's: commit 122f26726b5e16174bf8a707df14be1d93c49d62 Author: Henrique de Moraes Holschuh h...@hmh.eng.br Date: Mon Aug 9 23:48:18 2010 -0300 thinkpad-acpi: find ACPI video device by synthetic HID So I checked out that commit and found that it shouldn't work either, since it has the same problem of the current code. The ACPI video controller device is given an id of ACPI_VIDEO_HID, but it's only known to Linux ACPI, not ACPICA, so the function provided by ACPICA acpi_get_devices will not work in this case, as that function will really check the control method of _HID under the handle, which does not exist no matter if Linux ACPI has added an id to its device structure or not. OTOH, the function provided by Linux ACPI acpi_device_hid will see the added id. In a word, the add of the HID will not affect the ASL namespace layout of the device node(thus, no _HID control method will be added to the device node). It seems that this problem has been found previously by: commit ff413195e830541afeae469fc866ecd0319abd7e Author: Alex Hung alex.h...@canonical.com Date: Tue Apr 24 16:40:52 2012 +0800 thinkpad-acpi: fix issuing duplicated key events for brightness up/down The tp_features.bright_acpimode will not be set correctly for brightness control because ACPI_VIDEO_HID will not be located in ACPI. As a result, a duplicated key event will always be sent. acpi_video_backlight_support() is sufficient to detect standard ACPI brightness control. Signed-off-by: Alex Hung alex.h...@canonical.com Signed-off-by: Matthew Garrett m...@redhat.com Thanks, Aaron ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
The tpacpi_acpi_handle_locate function makes use of acpi_get_devices to
locate handle for ACPI video by HID, the problem is, ACPI video node
doesn't really have HID defined(i.e. no _HID control method is defined
for video device), so.. that function would fail. This can be solved by
enhancing the callback function for acpi_get_devices, where we can use
acpi_device_hid function to check if the ACPI node corresponds to a
video controller.
In addition to that, the _BCL control method only exists under a video
output device node, not a video controller device node. So to evaluate
_BCL, we need the handle of a video output device node, which is child
of the located video controller node from tpacpi_acpi_handle_locate.
The two fix are necessary for some Thinkpad models to emit notification
on backlight hotkey press as a result of evaluation of _BCL.
Signed-off-by: Aaron Lu aaron...@intel.com
Tested-by: Igor Gnatenko i.gnatenko.br...@gmail.com
---
drivers/platform/x86/thinkpad_acpi.c | 31 ---
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index 03ca6c1..170f278 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -700,6 +700,14 @@ static void __init drv_acpi_handle_init(const char *name,
static acpi_status __init tpacpi_acpi_handle_locate_callback(acpi_handle
handle,
u32 level, void *context, void **return_value)
{
+ struct acpi_device *dev;
+ if (!strcmp(context, video)) {
+ if (acpi_bus_get_device(handle, dev))
+ return AE_OK;
+ if (strcmp(ACPI_VIDEO_HID, acpi_device_hid(dev)))
+ return AE_OK;
+ }
+
*(acpi_handle *)return_value = handle;
return AE_CTRL_TERMINATE;
@@ -712,10 +720,10 @@ static void __init tpacpi_acpi_handle_locate(const char
*name,
acpi_status status;
acpi_handle device_found;
- BUG_ON(!name || !hid || !handle);
+ BUG_ON(!name || !handle);
vdbg_printk(TPACPI_DBG_INIT,
trying to locate ACPI handle for %s, using HID %s\n,
- name, hid);
+ name, hid ? hid : NULL);
memset(device_found, 0, sizeof(device_found));
status = acpi_get_devices(hid, tpacpi_acpi_handle_locate_callback,
@@ -6090,19 +6098,28 @@ static int __init tpacpi_query_bcl_levels(acpi_handle
handle)
{
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *obj;
+ struct acpi_device *device, *child;
int rc;
- if (ACPI_SUCCESS(acpi_evaluate_object(handle, _BCL, NULL, buffer))) {
+ if (acpi_bus_get_device(handle, device))
+ return 0;
+
+ rc = 0;
+ list_for_each_entry(child, device-children, node) {
+ acpi_status status = acpi_evaluate_object(child-handle, _BCL,
+ NULL, buffer);
+ if (ACPI_FAILURE(status))
+ continue;
+
obj = (union acpi_object *)buffer.pointer;
if (!obj || (obj-type != ACPI_TYPE_PACKAGE)) {
pr_err(Unknown _BCL data, please report this to %s\n,
- TPACPI_MAIL);
+ TPACPI_MAIL);
rc = 0;
} else {
rc = obj-package.count;
}
- } else {
- return 0;
+ break;
}
kfree(buffer.pointer);
@@ -6118,7 +6135,7 @@ static unsigned int __init
tpacpi_check_std_acpi_brightness_support(void)
acpi_handle video_device;
int bcl_levels = 0;
- tpacpi_acpi_handle_locate(video, ACPI_VIDEO_HID, video_device);
+ tpacpi_acpi_handle_locate(video, NULL, video_device);
if (video_device)
bcl_levels = tpacpi_query_bcl_levels(video_device);
--
1.8.4.12.g2ea3df6
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL
On Tue, 24 Sep 2013, Aaron Lu wrote: locate handle for ACPI video by HID, the problem is, ACPI video node doesn't really have HID defined(i.e. no _HID control method is defined ACPI video is supposed to attach a virtual HID node (ACPI_VIDEO_HID) to ACPI video devices so as to keep the non-trivial video device detection logic in just one place instead of reinventing the wheel in every driver (which is always a recipe for disaster). When did that break? -- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
