From patchwork Thu Dec 17 09:48:23 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 7871431 Return-Path: X-Original-To: patchwork-platform-driver-x86@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id F3155BEEE5 for ; Thu, 17 Dec 2015 09:48:41 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 98916203EB for ; Thu, 17 Dec 2015 09:48:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E2D43203E3 for ; Thu, 17 Dec 2015 09:48:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751403AbbLQJsi (ORCPT ); Thu, 17 Dec 2015 04:48:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36063 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbbLQJsc (ORCPT ); Thu, 17 Dec 2015 04:48:32 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 34675C0B9331; Thu, 17 Dec 2015 09:48:32 +0000 (UTC) Received: from shalem.localdomain (vpn1-5-85.ams2.redhat.com [10.36.5.85]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tBH9mO3k000400 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 17 Dec 2015 04:48:26 -0500 Subject: Re: Dell Vostro V131 hotkeys revisited To: =?UTF-8?B?TWljaGHFgiBLxJlwaWXFhA==?= References: <559C1C17.3060009@dell.com> <20150708035346.GB2413@eudyptula.hq.kempniu.pl> <20150722073513.GA2381@eudyptula.hq.kempniu.pl> <20150831095107.GA2397@eudyptula.hq.kempniu.pl> <20150910043812.GB108260@vmdeb7> <20151113101716.GA5458@eudyptula.hq.kempniu.pl> <20151207114320.GC13893@pali> <20151216090531.GA2425@eudyptula.hq.kempniu.pl> <20151216093057.GQ13531@pali> <56713CFE.8090201@redhat.com> <20151217080510.GA3534@eudyptula.hq.kempniu.pl> Cc: =?UTF-8?Q?Pali_Roh=c3=a1r?= , Darren Hart , Mario Limonciello , "Gowda, Srinivas G" , "Brown, Michael E" , "Warzecha, Douglas" , Matthew Garrett , "Kabir, Rezwanul" , Alex Hung , "platform-driver-x86@vger.kernel.org" From: Hans de Goede Message-ID: <567284E7.3050103@redhat.com> Date: Thu, 17 Dec 2015 10:48:23 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20151217080510.GA3534@eudyptula.hq.kempniu.pl> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 Sender: platform-driver-x86-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: platform-driver-x86@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Micha?, On 17-12-15 09:05, Micha? K?pie? wrote: > Hi Hans, > >> Pali, thanks for bringing this one to my attention, so as I see it we >> need to do a number of things: >> >> 1) Add a dmi based quirk to: drivers/acpi/video_detect.c to >> use native backlight on this model, so that the flickering gets fixed, >> for now you can use acpi_backlight=native for testing, until we've got >> the keys sorted out and then we should submit a kernel patch with this >> quirk >> >> 2a) I'm not familiar with the WMI bits, but as I see it we want that driver to >> be loaded to get other hotkeys to work, so lets load it (I assume this will >> already happen automatically if enabled). If I understand things correctly >> then doing this will silence the i8042 generated brightness key events, but >> it will cause the acpi-video bus generated key events lag in time by one >> event. So we will need an option in drivers/acpi/acpi_video.c to stop it >> from generating key-presses, this may be useful in some other (rare) cases >> too. I've written a patch for this (attached), can you build a kernel with >> this patch and then add "video.report_key_events=1" to the kernel cmdline >> and see if that helps ? > > Thank you for a quick response. Your patch works fine. If per-model > key event generation suppression is acceptable, I believe this would > indeed work around V131's issues. The elegance of this solution is that > it doesn't depend on the state of WMI event reporting - if it is off, > events will still be correctly reported by i8042; if it is on, dell-wmi > will generate them. Either way, the behavior will be correct. > >> Or alternatively we could not load the wmi driver, and fix the double events >> being reported by the i8042 / atkbd driver by adding a udev hwdb entry to >> filter these out, we already do that for some laptops because of similar issues, >> see e.g. this part of: /lib/udev/hwdb.d/60-keyboard.hwdb >> >> # Dell Inspiron 1520 >> evdev:atkbd:dmi:bvn*:bvr*:bd*:svnDell*:pnInspiron*1520:pvr* >> KEYBOARD_KEY_85=unknown # Brightness Down, also emitted by acpi-video, ignore >> KEYBOARD_KEY_86=unknown # Brightness Up, also emitted by acpi-video, ignore >> >> To test this you need to edit /lib/udev/hwdb.d/60-keyboard.hwdb add an entry >> for your laptop (see "cat /sys/class/dmi/id/product_name" output to find what to >> put after "pn" ), then rebuild the hwdb: "sudo udevadm hwdb --update" and then >> reboot, yes reboot there is another way to re-apply the rules but rebooting is >> really so much easier. >> >> I think you should try this method too and see if the flickering goes away >> when fixing the double events, without needing to use acpi_backlight=native. > > The flickering is caused by two things happening in short succession: > > * an erroneous ASLE request, which is only ignored by the i915 driver > with acpi_backlight=native, > > * a notification sent to the ACPI video driver, which causes > brightness to be switched after HZ / 10 with acpi_backlight=video. > > So I believe those double events are just a side effect, not the root > cause of the flickering. > >> It is also not clear to me if you've tried using the WMI events without >> acpi_backlight=native, maybe that fixes things magically ? > > In order for WMI events to be reported at all on Vostro V131, a special > SMI has to be invoked. That SMI always messes up the key events > reported by the ACPI video driver, no matter what acpi_backlight is set > to. And the flickering is caused by yet another piece of faulty AML > inside the CESM method. Is this what you were asking? If not, I'll be > happy to answer any further questions you might have. > > Also, I need to apologize. While testing your patch I noticed that my > notes got mixed up at some point, rendering the story in my previous > message partially invalid. I wrote that in order for dell-wmi to report > key events, acpi_backlight has to be set to "native". That is not true > as it has to be set to "vendor", as the code in dell_wmi_init() and > dell_wmi_process_key() clearly shows. > > To clear things up, here is the current state of affairs: > > * acpi_backlight=native solves the flickering issue, but doesn't help > with key event "lagging" after WMI is enabled, > > * using the patch provided by Hans (or a similar one), the "lagged" > events can be filtered, leaving key event generation to dell-wmi, > > * dell-wmi won't generate events unless acpi_backlight=vendor, which > in turn breaks brightness control due to faulty ASLE requests. > > To break out of this loop, I suggest that: > > * acpi_backlight should default to "native" for Vostro V131, based on > a DMI check, Ack. > * brightness key event generation performed by the ACPI video driver > should always be suppressed on Vostro V131, because it's faulty due > to firmware bugs (and correct events will be reported anyway by > either i8042 or WMI), s/should always be suppressed/should be suppressed by default/ otherwise ack. We can easily do this the same way we currently deal with the disable_backlight_sysfs_if option in acpi_video.c > * yet another quirk should be added to dell-wmi, so that brightness > key events are generated not only when acpi_backlight=vendor, but > also when acpi_backlight=native. Nack, the real problem here is that checking if acpi_backlight!=vendor is the wrong way to check if key event generation should be suppressed. This actually reminds me that I've the following item on my todo list for a while but I've not gotten around to it yet: -Add an acpi_video_handles_key_presses() helper, and use this where appropriate (dell-wmi and others). The mean reason for that item on my todo list was to make code doing brigthness key-event suppression more readable. But we can also use it for this case, if we check the new video.report_key_events parameter in the acpi_video_handles_key_presses() helper, and switch dell-wmi over to this helper, things will just work without needing yet another quirk in dell-wmi :) I've written a patch-set implementing this (attached), this obsoletes my previous patch. As before, please test this with: acpi_backlight=native video.report_key_events=1 On the kernel cmdline, we can add a patch adding dmi quirks to make those the default later. For that patch I'm going to need the dmi strings for your laptop, can you please do: for i in /sys/class/dmi/id/*_*; do echo $i; cat $i; done And then include the output in your next mail ? Feel free to leave out the serial numbers, asset tags and uuids, those are potentially privacy sensitive and I don't need them. > The only downside I see to such a solution is that by default the user > would have to use a userspace helper in order for the key events to be > translated into brightness changes. However, if they so desire, > acpi_backlight may still be set to "video", which would enable > brightness changes to be done by the kernel, though with the flickering > effect still in place. Sounds like a fair choice to me. What do you > think? I do not see the need for a userspace helper as a problem, this actually is the case on most modern laptops already, as we default to acpi_backlight=native there, and that has the same requirement. Regards, Hans From 5e7ff99407aeb60c2f1516cdd80d7859646497dd Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 16 Dec 2015 11:19:00 +0100 Subject: [PATCH 4/4] acpi-video: Add a module option to disable the reporting of keypresses Add a module option to disable the reporting of keypresses, in some buggy firmware implementatinon, the reported events are wrong. E.g. they lag reality by one event in the case triggering the writing of this patch. In this case it is better to not forward these wrong events to userspace (esp.) when there is another source of the same events which is not buggy. Note this is only intended to work around implementations which send events which are plain wrong. In some cases we get double events, e.g. from both acpi-video and the atkbd driver, in this case acpi-video is considered the canonical source, and the events from the other source should be filtered (using e.g. /lib/udev/hwdb.d/60-keyboard.hwdb). Signed-off-by: Hans de Goede --- drivers/acpi/acpi_video.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 2a649f3e..2971154 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -77,6 +77,13 @@ module_param(allow_duplicates, bool, 0644); static int disable_backlight_sysfs_if = -1; module_param(disable_backlight_sysfs_if, int, 0444); +#define REPORT_OUTPUT_KEY_EVENTS 0x01 +#define REPORT_BRIGHTNESS_KEY_EVENTS 0x02 +static int report_key_events = -1; +module_param(report_key_events, int, 0644); +MODULE_PARM_DESC(report_key_events, + "0: none, 1: output changes, 2: brightness changes, 3: all"); + static bool device_id_scheme = false; module_param(device_id_scheme, bool, 0444); @@ -1480,7 +1487,7 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) /* Something vetoed the keypress. */ keycode = 0; - if (keycode) { + if (keycode && (report_key_events & REPORT_OUTPUT_KEY_EVENTS)) { input_report_key(input, keycode, 1); input_sync(input); input_report_key(input, keycode, 0); @@ -1544,7 +1551,7 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data) acpi_notifier_call_chain(device, event, 0); - if (keycode) { + if (keycode && (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS)) { input_report_key(input, keycode, 1); input_sync(input); input_report_key(input, keycode, 0); @@ -2080,7 +2087,8 @@ bool acpi_video_handles_brightness_key_presses(void) have_video_busses = !list_empty(&video_bus_head); mutex_unlock(&video_list_lock); - return have_video_busses; + return have_video_busses && + (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS); } EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses); -- 2.5.0