Dell Vostro V131 hotkeys revisited

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

Dell Vostro V131 hotkeys revisited

Commit Message

Comments

Patch