| Message ID | 1380016052-15315-5-git-send-email-aaron.lu@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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?
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 <hmh@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.hung@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.hung@canonical.com> Signed-off-by: Matthew Garrett <mjg@redhat.com> Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <hmh@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.hung@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.hung@canonical.com> > Signed-off-by: Matthew Garrett <mjg@redhat.com> Until that. And unfortunately I did not connect the dots at the time. Thanks for explaining the issue properly.
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.lu@intel.com> > Tested-by: Igor Gnatenko <i.gnatenko.brain@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 <hmh@hmh.eng.br>
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,
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.
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,
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);