From: Stefan Seyfried <[email protected]>
commit ed83c9171829 broke the hotkeys on my Toughbook CF-51.
I'm questioning the general validity of that commit, but as I only
have a single machine to test, add a module parameter to allow making
it work at runtime.
Fixes: ed83c9171829 platform/x86: panasonic-laptop: Resolve hotkey double trigger bug
Signed-off-by: Stefan Seyfried <[email protected]>
---
drivers/platform/x86/panasonic-laptop.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index ca6137f4000f..83acae75aee2 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -141,6 +141,9 @@ MODULE_AUTHOR("Martin Lucina <[email protected]>");
MODULE_AUTHOR("Kenneth Chan <[email protected]>");
MODULE_DESCRIPTION("ACPI HotKey driver for Panasonic Let's Note laptops");
MODULE_LICENSE("GPL");
+static bool hotkey_input;
+module_param(hotkey_input, bool, 0644);
+MODULE_PARM_DESC(hotkey_input, "Send all hotkeys to the input subsystem");
#define LOGPREFIX "pcc_acpi: "
@@ -785,7 +788,7 @@ static void acpi_pcc_generate_keyinput(struct pcc_acpi *pcc)
}
/* for the magic values, see panasonic_keymap[] above */
- if (key == 7 || key == 9 || key == 10) {
+ if (hotkey_input || key == 7 || key == 9 || key == 10) {
if (!sparse_keymap_report_event(hotk_input_dev,
key, updown, false))
pr_err("Unknown hotkey event: 0x%04llx\n", result);
--
2.36.1
On Sun, Jun 12, 2022 at 3:54 PM <[email protected]> wrote:
>
> From: Stefan Seyfried <[email protected]>
>
> commit ed83c9171829 broke the hotkeys on my Toughbook CF-51.
The commit
> I'm questioning the general validity of that commit, but as I only
> have a single machine to test, add a module parameter to allow making
> it work at runtime.
Thanks for the report and the fix, my comments below.
> +static bool hotkey_input;
> +module_param(hotkey_input, bool, 0644);
> +MODULE_PARM_DESC(hotkey_input, "Send all hotkeys to the input subsystem");
We usually add module options in very bad cases where it's very useful
for debugging or when some devices require the opposite settings while
can't be distinguished automatically. Here I do not see either of such
cases. Hence, I would prefer to see a DMI based quirk as it's done a
lot in the PDx86 drivers. Can you do that?
--
With Best Regards,
Andy Shevchenko
I'm resending it due to reported spam. I forgot to turn off HTML
formatting. My bad.
Thanks for the patches.
> From: Stefan Seyfried <[email protected]>
>
> commit ed83c9171829 broke the hotkeys on my Toughbook CF-51.
> I'm questioning the general validity of that commit, but as I only
> have a single machine to test, add a module parameter to allow making
> it work at runtime.
I can confirm that as soon as the hotkey_input option is enabled (at
least on my aged CF-W5) it reports the ACPI event twice. Unfortunately
it's the only machine I have for testing. Unless we have a bigger
sample base, making it a module_param is a safe choice.
Otherwise the patches look good to me.
Reviewed-by: Kenneth Chan <[email protected]>
--
Kenneth
On Wed, Jun 15, 2022 at 1:21 PM Andy Shevchenko
<[email protected]> wrote:
> On Sun, Jun 12, 2022 at 3:54 PM <[email protected]> wrote:
> We usually add module options in very bad cases where it's very useful
> for debugging or when some devices require the opposite settings while
> can't be distinguished automatically. Here I do not see either of such
> cases. Hence, I would prefer to see a DMI based quirk as it's done a
> lot in the PDx86 drivers. Can you do that?
Looking into the code of the culprit patch, have you tried to add a
debug pr_info() and see what value is in the result? Perhaps you may
just sort out by correcting that.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 15.06.22 13:24, Andy Shevchenko wrote:
> On Wed, Jun 15, 2022 at 1:21 PM Andy Shevchenko
> <[email protected]> wrote:
>> On Sun, Jun 12, 2022 at 3:54 PM <[email protected]> wrote:
>
>> We usually add module options in very bad cases where it's very useful
>> for debugging or when some devices require the opposite settings while
>> can't be distinguished automatically. Here I do not see either of such
>> cases. Hence, I would prefer to see a DMI based quirk as it's done a
>> lot in the PDx86 drivers. Can you do that?
I can do that, but... below ;-)
> Looking into the code of the culprit patch, have you tried to add a
> debug pr_info() and see what value is in the result? Perhaps you may
> just sort out by correcting that.
The driver is working fine, it's just that Kenneth's machine is getting
most of the hotkey events (I'd guess all but sleep, hibernate, battery)
twice. That's why he disabled the key generation from the panasonic_acpi
driver for them. (My guess is that on his CF-W5, they are also coming in
via normal keyboard input path). My CF-51 does only generate them via
acpi, so if they are not generated, I get nothing.
Hence the module parameter so that the two known users of this module
(Kenneth and me) can adjust this to their needs.
Now about the DMI match: I can do that.
But let's face it: the panasonic laptops are pretty rare in the wild, so
even if I'm "whitelisting" the CF-51, then probably other models will
need the same treatment and we have no real way of finding out which
ones, unless people complain. (For example my CF-51 is about 17 years
old now and I just pulled it out and updated it to the latest and
greatest "because I can". That's also why it has taken me so long to
even notice the driver was broken for me. So people not complaining will
not mean "nothing is broken" but rather "this code has not many users").
So even if I add the DMI match (which is a good idea anyhow because then
"my" model will work out of the box, while right now I need to add a
module parameter or switch it on later), I'd still vote for having a
possibility for overriding the DMI results.
Would that be OK?
Best regards,
Stefan
--
Stefan Seyfried
"For a successful technology, reality must take precedence over
public relations, for nature cannot be fooled." -- Richard Feynman
Hi Stefan,
On 6/15/22 19:10, Stefan Seyfried wrote:
> Hi Andy,
>
> On 15.06.22 13:24, Andy Shevchenko wrote:
>> On Wed, Jun 15, 2022 at 1:21 PM Andy Shevchenko
>> <[email protected]> wrote:
>>> On Sun, Jun 12, 2022 at 3:54 PM <[email protected]> wrote:
>>
>>> We usually add module options in very bad cases where it's very useful
>>> for debugging or when some devices require the opposite settings while
>>> can't be distinguished automatically. Here I do not see either of such
>>> cases. Hence, I would prefer to see a DMI based quirk as it's done a
>>> lot in the PDx86 drivers. Can you do that?
>
> I can do that, but... below ;-)
>
>> Looking into the code of the culprit patch, have you tried to add a
>> debug pr_info() and see what value is in the result? Perhaps you may
>> just sort out by correcting that.
>
> The driver is working fine, it's just that Kenneth's machine is getting most of the hotkey events (I'd guess all but sleep, hibernate, battery) twice. That's why he disabled the key generation from the panasonic_acpi driver for them. (My guess is that on his CF-W5, they are also coming in via normal keyboard input path). My CF-51 does only generate them via acpi, so if they are not generated, I get nothing.
> Hence the module parameter so that the two known users of this module (Kenneth and me) can adjust this to their needs.
>
> Now about the DMI match: I can do that.
> But let's face it: the panasonic laptops are pretty rare in the wild, so even if I'm "whitelisting" the CF-51, then probably other models will need the same treatment and we have no real way of finding out which ones, unless people complain. (For example my CF-51 is about 17 years old now and I just pulled it out and updated it to the latest and greatest "because I can". That's also why it has taken me so long to even notice the driver was broken for me. So people not complaining will not mean "nothing is broken" but rather "this code has not many users").
> So even if I add the DMI match (which is a good idea anyhow because then "my" model will work out of the box, while right now I need to add a module parameter or switch it on later), I'd still vote for having a possibility for overriding the DMI results.
> Would that be OK?
Actually I agree with your original assessment that Kenneth's patch
(ed83c9171829) which broke things on your laptop is wrong.
Back then I did not properly realize that it is effectively
disabling event generation for most of the reported event codes.
If anything there should be a DMI match for Kenneth's model and
reporting the events normally should be the default.
Kenneth, can you check with e.g. evemu-record or evtest
where the double events are coming from ? Obviously one of
the events is coming from the panasonic-laptop driver, but
where is the other event coming from. Is it coming from the
atkbd driver; or ... ? Maybe from the acpi-video driver
for the brightness keys ?
Regards,
Hans
On Thu, 16 Jun 2022 at 03:28, Hans de Goede <[email protected]> wrote:
>
> Kenneth, can you check with e.g. evemu-record or evtest
> where the double events are coming from ? Obviously one of
> the events is coming from the panasonic-laptop driver, but
> where is the other event coming from. Is it coming from the
> atkbd driver; or ... ? Maybe from the acpi-video driver
> for the brightness keys ?
>
Certainly. I'm happy to dig deeper and see what it's up to.
--
Kenneth
On Wed, Jun 15, 2022 at 9:28 PM Hans de Goede <[email protected]> wrote:
> On 6/15/22 19:10, Stefan Seyfried wrote:
...
> If anything there should be a DMI match for Kenneth's model and
> reporting the events normally should be the default.
You beat me up to this comment. I was about to answer something similar.
--
With Best Regards,
Andy Shevchenko
On Thu, 16 Jun 2022 at 03:28, Hans de Goede <[email protected]> wrote:
>
> Kenneth, can you check with e.g. evemu-record or evtest
> where the double events are coming from ? Obviously one of
> the events is coming from the panasonic-laptop driver, but
> where is the other event coming from. Is it coming from the
> atkbd driver; or ... ? Maybe from the acpi-video driver
> for the brightness keys ?
>
Here's the evtest results:
acpi-video driver generates KEY_BRIGHTNESSDOWN, KEY_BRIGHTNESSUP
atkbd generates KEY_MUTE, KEY_VOLUMEUP, KEY_VOLUMEDOWN
Hotkey_input=Y (i.e. before patch ed83c9171829)
panasonic-laptop driver generates all keys, i.e. KEY_SLEEP,
KEY_BATTERY, KEY_SUSPEND plus all the above keys
hotkey_input=N
panasonic-laptop driver generates KEY_SLEEP, KEY_BATTERY and KEY_SUSPEND
So the duplicated brightness and volume key events come from the atkbd
and acpi-video drivers on my CF-W5. I haven't looked at the other
platform drivers. I'm wondering if they honour atkbd and acpi-driver
events in a case like this, or just report everything.
Attached is the dmidecode of my CF-W5, just to be verbose.
> Hence the module parameter so that the two known users of this module (Kenneth and me) can adjust this to their needs.
>
> Now about the DMI match: I can do that.
> But let's face it: the panasonic laptops are pretty rare in the wild, so even if I'm "whitelisting" the CF-51, then probably other models will need the same treatment and we have no real way of finding out which ones, unless people complain.
> So even if I add the DMI match (which is a good idea anyhow because then "my" model will work out of the box, while right now I need to add a module parameter or switch it on later), I'd still vote for having a possibility for overriding the DMI results.
In an ideal world, more panasonic-laptop users will send us their
DSDT. I think the most uptodate model has a MAT0035 device_id (it
increments for each generation) while our driver is at the very
outdated MAT0021. But before it happens, I agree with Stefan on that
point.
--
Kenneth
Hi,
On 6/17/22 09:51, Kenneth Chan wrote:
> On Thu, 16 Jun 2022 at 03:28, Hans de Goede <[email protected]> wrote:
>>
>> Kenneth, can you check with e.g. evemu-record or evtest
>> where the double events are coming from ? Obviously one of
>> the events is coming from the panasonic-laptop driver, but
>> where is the other event coming from. Is it coming from the
>> atkbd driver; or ... ? Maybe from the acpi-video driver
>> for the brightness keys ?
>>
>
> Here's the evtest results:
>
> acpi-video driver generates KEY_BRIGHTNESSDOWN, KEY_BRIGHTNESSUP
> atkbd generates KEY_MUTE, KEY_VOLUMEUP, KEY_VOLUMEDOWN
>
> Hotkey_input=Y (i.e. before patch ed83c9171829)
> panasonic-laptop driver generates all keys, i.e. KEY_SLEEP,
> KEY_BATTERY, KEY_SUSPEND plus all the above keys
>
> hotkey_input=N
> panasonic-laptop driver generates KEY_SLEEP, KEY_BATTERY and KEY_SUSPEND
>
> So the duplicated brightness and volume key events come from the atkbd
> and acpi-video drivers on my CF-W5. I haven't looked at the other
> platform drivers. I'm wondering if they honour atkbd and acpi-driver
> events in a case like this, or just report everything.
Thank you for providing this info. Can you please give
the attached patch series a try, this includes Stefan's 1/2 patch
and replaces Stefan's 2/2 patch.
This will hopefully fix the double key-presses for you, while
also keeping everything working for Stefan without requiring
a module option or DMI quirks.
Stefan can you also give this series a try please?
###
Looking at this has also brought up an unrelated backlight question:
Kenneth, since you have acpi-video reporting keypresses you will
likely also have an acpi_video (or perhaps a native intel) backlight
under /sys/class/backlight and I noticed that panasonic-laptop
uncondirionally registers its backlight so you may very well end
up with 2 backlight controls under /sys/class/backlight, which
we generally try to avoid (so that userspace does not have to
guess which one to use).
Can you do:
ls /sys/class/backlight
and let me know the output?
Also if there are 2 backlights there then please do:
cat /sys/class/backlight/<name>/max_brightness
to find out the range (0-value)
and then try if they both work by doing:
echo $number > /sys/class/backlight/<name>/brightness
with different $number values in the range and see
if this actually changes the brightness.
While we are at it, Stefan can you do the same please?
Regards,
Hans
>
> Attached is the dmidecode of my CF-W5, just to be verbose.
>
>> Hence the module parameter so that the two known users of this module (Kenneth and me) can adjust this to their needs.
>>
>> Now about the DMI match: I can do that.
>> But let's face it: the panasonic laptops are pretty rare in the wild, so even if I'm "whitelisting" the CF-51, then probably other models will need the same treatment and we have no real way of finding out which ones, unless people complain.
>> So even if I add the DMI match (which is a good idea anyhow because then "my" model will work out of the box, while right now I need to add a module parameter or switch it on later), I'd still vote for having a possibility for overriding the DMI results.
>
> In an ideal world, more panasonic-laptop users will send us their
> DSDT. I think the most uptodate model has a MAT0035 device_id (it
> increments for each generation) while our driver is at the very
> outdated MAT0021. But before it happens, I agree with Stefan on that
> point.
>
Hi Hans,
On 17.06.22 13:07, Hans de Goede wrote:
> Thank you for providing this info. Can you please give
> the attached patch series a try, this includes Stefan's 1/2 patch
> and replaces Stefan's 2/2 patch.
>
> This will hopefully fix the double key-presses for you, while
> also keeping everything working for Stefan without requiring
> a module option or DMI quirks.
>
> Stefan can you also give this series a try please?
Works for me, almost out of the box.
I need to enable "report_key_events=1" in the video module, then the
panasonic-acpi module starts reporting brightness up/down keys.
Volume and mute keys work without manual changes.
(I tested against 5.18.2 because that's what was already prepared. That
old machine takes quite some time, even to just compile the platform/x86
subdirectory ;-) but I don't think this is relevant. If you think it is,
I can also test against latest 5.19-rc code)
> Looking at this has also brought up an unrelated backlight question:
>
> Kenneth, since you have acpi-video reporting keypresses you will
> likely also have an acpi_video (or perhaps a native intel) backlight
> under /sys/class/backlight and I noticed that panasonic-laptop
> uncondirionally registers its backlight so you may very well end
> up with 2 backlight controls under /sys/class/backlight, which
> we generally try to avoid (so that userspace does not have to
> guess which one to use).
>
> Can you do:
> ls /sys/class/backlight
toughbook:~ # ls -l /sys/class/backlight/
total 0
lrwxrwxrwx 1 root root 0 Jun 17 14:45 intel_backlight ->
../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
lrwxrwxrwx 1 root root 0 Jun 17 14:49 panasonic ->
../../devices/virtual/backlight/panasonic
> and let me know the output?
>
> Also if there are 2 backlights there then please do:
> cat /sys/class/backlight/<name>/max_brightness
> to find out the range (0-value)
toughbook:/sys/class/backlight # grep . */max_brightness
intel_backlight/max_brightness:19531
panasonic/max_brightness:255
> and then try if they both work by doing:
>
> echo $number > /sys/class/backlight/<name>/brightness
>
> with different $number values in the range and see
> if this actually changes the brightness.
intel_backlight: does not work
panasonic: does work
> While we are at it, Stefan can you do the same please?
See above.
But hey, this is an i855GM graphics chip, I'm happy if it is still
working *at all* (for example I need to avoid the xf86-intel driver and
use the modesetting driver instead to get a usable sytstem)
And I'm totally happy if all I have to do in the future is a
option video report_key_events=1
modprobe.conf file ;-)
Best regards,
Stefan
--
Stefan Seyfried
"For a successful technology, reality must take precedence over
public relations, for nature cannot be fooled." -- Richard Feynman
Hi,
Thank you for the quick testing.
On 6/17/22 15:07, Stefan Seyfried wrote:
> Hi Hans,
>
> On 17.06.22 13:07, Hans de Goede wrote:
>
>> Thank you for providing this info. Can you please give
>> the attached patch series a try, this includes Stefan's 1/2 patch
>> and replaces Stefan's 2/2 patch.
>>
>> This will hopefully fix the double key-presses for you, while
>> also keeping everything working for Stefan without requiring
>> a module option or DMI quirks.
>>
>> Stefan can you also give this series a try please?
>
> Works for me, almost out of the box.
> I need to enable "report_key_events=1" in the video module, then the panasonic-acpi module starts reporting brightness up/down keys.
Ok, so you need another module option that is not really helpful.
The idea behind the acpi_video_handles_brightness_key_presses() check
is that if the ACPI video bus device is present it is expected to
already report brightness up/down keypresses and we want to avoid
duplicates.
Can you check with evtest or evemu-record that the brightness
events are not already being delivered by the "Video Bus"
input device ?
> Volume and mute keys work without manual changes.
Good.
> (I tested against 5.18.2 because that's what was already prepared. That old machine takes quite some time, even to just compile the platform/x86 subdirectory ;-) but I don't think this is relevant. If you think it is, I can also test against latest 5.19-rc code)
Testing against 5.18 is fine .
>> Looking at this has also brought up an unrelated backlight question:
>>
>> Kenneth, since you have acpi-video reporting keypresses you will
>> likely also have an acpi_video (or perhaps a native intel) backlight
>> under /sys/class/backlight and I noticed that panasonic-laptop
>> uncondirionally registers its backlight so you may very well end
>> up with 2 backlight controls under /sys/class/backlight, which
>> we generally try to avoid (so that userspace does not have to
>> guess which one to use).
>>
>> Can you do:
>> ls /sys/class/backlight
>
> toughbook:~ # ls -l /sys/class/backlight/
> total 0
> lrwxrwxrwx 1 root root 0 Jun 17 14:45 intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
> lrwxrwxrwx 1 root root 0 Jun 17 14:49 panasonic -> ../../devices/virtual/backlight/panasonic
>
>> and let me know the output?
>>
>> Also if there are 2 backlights there then please do:
>> cat /sys/class/backlight/<name>/max_brightness
>> to find out the range (0-value)
>
> toughbook:/sys/class/backlight # grep . */max_brightness
> intel_backlight/max_brightness:19531
> panasonic/max_brightness:255
>
>> and then try if they both work by doing:
>>
>> echo $number > /sys/class/backlight/<name>/brightness
>>
>> with different $number values in the range and see
>> if this actually changes the brightness.
>
> intel_backlight: does not work
> panasonic: does work
Ok, so that suggests that the ACPI video bus on your
device is defunct, so I guess it also does not report
key-presses (see above) ?
This will also need some work then because we want to move
to there only being 1 (actually working) backlight-class
device. Rather then having multiple and let userspace
guess which one it needs to use.
>> While we are at it, Stefan can you do the same please?
>
> See above.
> But hey, this is an i855GM graphics chip, I'm happy if it is still working *at all* (for example I need to avoid the xf86-intel driver and use the modesetting driver instead to get a usable sytstem)
>
> And I'm totally happy if all I have to do in the future is a
>
> option video report_key_events=1
>
> modprobe.conf file ;-)
We really don't want people to have to specify module-options just
to have things working.
Stefam, at least for the backlight class-device issue we will need a DMI
quirk, so can you run:
sudo dmidecode > dmidecode.txt
and then attach the output to your next email, or send me a copy
privately ?
Regards,
Hans
It took quite a while to do a full compile, just to be safe.
On Fri, 17 Jun 2022 at 19:07, Hans de Goede <[email protected]> wrote:
>
>
> Looking at this has also brought up an unrelated backlight question:
>
> Kenneth, since you have acpi-video reporting keypresses you will
> likely also have an acpi_video (or perhaps a native intel) backlight
> under /sys/class/backlight and I noticed that panasonic-laptop
> uncondirionally registers its backlight so you may very well end
> up with 2 backlight controls under /sys/class/backlight, which
> we generally try to avoid (so that userspace does not have to
> guess which one to use).
>
> Can you do:
> ls /sys/class/backlight
root@jaguar:~# ls -l /sys/class/backlight/
total 0
lrwxrwxrwx 1 root root 0 Jun 19 17:26 acpi_video0 ->
../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0/
lrwxrwxrwx 1 root root 0 Jun 19 17:26 intel_backlight ->
../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight/
lrwxrwxrwx 1 root root 0 Jun 19 18:48 panasonic ->
../../devices/virtual/backlight/panasonic/
>
> and let me know the output?
>
> Also if there are 2 backlights there then please do:
> cat /sys/class/backlight/<name>/max_brightness
> to find out the range (0-value)
root@jaguar:~# cat /sys/class/backlight/acpi_video0/max_brightness
20
root@jaguar:~# cat /sys/class/backlight/intel_backlight/max_brightness
6375000
root@jaguar:~# cat /sys/class/backlight/panasonic/max_brightness
21
>
> and then try if they both work by doing:
>
> echo $number > /sys/class/backlight/<name>/brightness
>
> with different $number values in the range and see
> if this actually changes the brightness.
/sys/class/backlight/acpi_video0/brightness: works
/sys/class/backlight/intel_backlight/brightness: works
/sys/class/backlight/panasonic/brightness: does not work
The mute, volume up/down keys are still duplicated by atkbd after
applying 0005-platform-x86-panasonic-laptop-filter-out-duplicate-v.patch.
--
Kenneth
Hi Hans,
On 20.06.22 17:08, Hans de Goede wrote:
> Hi,
>
> Thank you for the quick testing.
>
> On 6/17/22 15:07, Stefan Seyfried wrote:
>> Hi Hans,
>>
>> On 17.06.22 13:07, Hans de Goede wrote:
>>
>>> Thank you for providing this info. Can you please give
>>> the attached patch series a try, this includes Stefan's 1/2 patch
>>> and replaces Stefan's 2/2 patch.
>>>
>>> This will hopefully fix the double key-presses for you, while
>>> also keeping everything working for Stefan without requiring
>>> a module option or DMI quirks.
>>>
>>> Stefan can you also give this series a try please?
>>
>> Works for me, almost out of the box.
>> I need to enable "report_key_events=1" in the video module, then the panasonic-acpi module starts reporting brightness up/down keys.
>
> Ok, so you need another module option that is not really helpful.
Well, I looked into the acpi_video.c module and that one is to blame.
By default, it assumes that both "OUTPUT_KEY_EVENTS" and
"BRIGHTNESS_KEY_EVENTS" should be handled by this module.
But on the CF-51, this does not happen. "Video Bus" does not generate
any key events (I'm not sure about output, but plugging in a VGA monitor
and enabling/disabling it with xrandr or tapping the "display" fn-f3
hotkey does not get anything from "Video Bus" input device.
> The idea behind the acpi_video_handles_brightness_key_presses() check
> is that if the ACPI video bus device is present it is expected to
> already report brightness up/down keypresses and we want to avoid
> duplicates.
Yes, but the check apparently returns true in my case, because:
return have_video_busses &&
(report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
apparently i have a video_bus ;-) and report_key_events (== -1) & 2 is true.
This check is totally fine, it's just that *the acpi_video.c* code is
missing a DMI match for my machine telling it that it handles nothing at
all.
> Can you check with evtest or evemu-record that the brightness
> events are not already being delivered by the "Video Bus"
> input device ?
I did, nothing at all gets delivered by Video Bus.
>> Volume and mute keys work without manual changes.
>
> Good.
>
>> (I tested against 5.18.2 because that's what was already prepared. That old machine takes quite some time, even to just compile the platform/x86 subdirectory ;-) but I don't think this is relevant. If you think it is, I can also test against latest 5.19-rc code)
>
> Testing against 5.18 is fine .
>
>>> Looking at this has also brought up an unrelated backlight question:
>>>
>>> Kenneth, since you have acpi-video reporting keypresses you will
>>> likely also have an acpi_video (or perhaps a native intel) backlight
>>> under /sys/class/backlight and I noticed that panasonic-laptop
>>> uncondirionally registers its backlight so you may very well end
>>> up with 2 backlight controls under /sys/class/backlight, which
>>> we generally try to avoid (so that userspace does not have to
>>> guess which one to use).
>>>
>>> Can you do:
>>> ls /sys/class/backlight
>>
>> toughbook:~ # ls -l /sys/class/backlight/
>> total 0
>> lrwxrwxrwx 1 root root 0 Jun 17 14:45 intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
>> lrwxrwxrwx 1 root root 0 Jun 17 14:49 panasonic -> ../../devices/virtual/backlight/panasonic
>>
>>> and let me know the output?
>>>
>>> Also if there are 2 backlights there then please do:
>>> cat /sys/class/backlight/<name>/max_brightness
>>> to find out the range (0-value)
>>
>> toughbook:/sys/class/backlight # grep . */max_brightness
>> intel_backlight/max_brightness:19531
>> panasonic/max_brightness:255
>>
>>> and then try if they both work by doing:
>>>
>>> echo $number > /sys/class/backlight/<name>/brightness
>>>
>>> with different $number values in the range and see
>>> if this actually changes the brightness.
>>
>> intel_backlight: does not work
>> panasonic: does work
>
> Ok, so that suggests that the ACPI video bus on your
> device is defunct, so I guess it also does not report
> key-presses (see above) ?
Yes, it looks like ACPI video driver is totally useless on that machine.
> This will also need some work then because we want to move
> to there only being 1 (actually working) backlight-class
> device. Rather then having multiple and let userspace
> guess which one it needs to use.
Well, the non-working backlight is coming from the i915 driver, but as
this is a very old Chipset (i855 GM) I'd rather be happy it works at all
instead of complaining ;-)
(I have another machine of similar age, hp nc6000 with ati graphics, and
there is no way getting it to work somewhat reliably at all)
>>> While we are at it, Stefan can you do the same please?
>>
>> See above.
>> But hey, this is an i855GM graphics chip, I'm happy if it is still working *at all* (for example I need to avoid the xf86-intel driver and use the modesetting driver instead to get a usable sytstem)
>>
>> And I'm totally happy if all I have to do in the future is a
>>
>> option video report_key_events=1
>>
>> modprobe.conf file ;-)
>
> We really don't want people to have to specify module-options just
> to have things working.
I understand, but then it's my job to get that DMI match to set this
parameter into acpi_video.c ;-)
> Stefam, at least for the backlight class-device issue we will need a DMI
> quirk, so can you run:
>
> sudo dmidecode > dmidecode.txt
>
> and then attach the output to your next email, or send me a copy
> privately ?
I'll send it privately as it is pretty big, but I think
DMI_BOARD_VENDOR, "Matsushita Electric Industrial Co.,Ltd."
DMI_BOARD_NAME, "CF51-1L"
(Similar to the CF51-2L in acpi/sleep.c) will do.
Best regards,
Stefan
--
Stefan Seyfried
"For a successful technology, reality must take precedence over
public relations, for nature cannot be fooled." -- Richard Feynman
Hi,
On 6/20/22 20:10, Stefan Seyfried wrote:
> Hi Hans,
>
> On 20.06.22 17:08, Hans de Goede wrote:
>> Hi,
>>
>> Thank you for the quick testing.
>>
>> On 6/17/22 15:07, Stefan Seyfried wrote:
>>> Hi Hans,
>>>
>>> On 17.06.22 13:07, Hans de Goede wrote:
>>>
>>>> Thank you for providing this info. Can you please give
>>>> the attached patch series a try, this includes Stefan's 1/2 patch
>>>> and replaces Stefan's 2/2 patch.
>>>>
>>>> This will hopefully fix the double key-presses for you, while
>>>> also keeping everything working for Stefan without requiring
>>>> a module option or DMI quirks.
>>>>
>>>> Stefan can you also give this series a try please?
>>>
>>> Works for me, almost out of the box.
>>> I need to enable "report_key_events=1" in the video module, then the panasonic-acpi module starts reporting brightness up/down keys.
>>
>> Ok, so you need another module option that is not really helpful.
>
> Well, I looked into the acpi_video.c module and that one is to blame.
> By default, it assumes that both "OUTPUT_KEY_EVENTS" and "BRIGHTNESS_KEY_EVENTS" should be handled by this module.
> But on the CF-51, this does not happen. "Video Bus" does not generate any key events (I'm not sure about output, but plugging in a VGA monitor and enabling/disabling it with xrandr or tapping the "display" fn-f3 hotkey does not get anything from "Video Bus" input device.
>
>> The idea behind the acpi_video_handles_brightness_key_presses() check
>> is that if the ACPI video bus device is present it is expected to
>> already report brightness up/down keypresses and we want to avoid
>> duplicates.
>
> Yes, but the check apparently returns true in my case, because:
>
> return have_video_busses &&
> (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
>
> apparently i have a video_bus ;-) and report_key_events (== -1) & 2 is true.
>
> This check is totally fine, it's just that *the acpi_video.c* code is missing a DMI match for my machine telling it that it handles nothing at all.
>
>> Can you check with evtest or evemu-record that the brightness
>> events are not already being delivered by the "Video Bus"
>> input device ?
>
> I did, nothing at all gets delivered by Video Bus.
>
>>> Volume and mute keys work without manual changes.
>>
>> Good.
>>
>>> (I tested against 5.18.2 because that's what was already prepared. That old machine takes quite some time, even to just compile the platform/x86 subdirectory ;-) but I don't think this is relevant. If you think it is, I can also test against latest 5.19-rc code)
>>
>> Testing against 5.18 is fine .
>>
>>>> Looking at this has also brought up an unrelated backlight question:
>>>>
>>>> Kenneth, since you have acpi-video reporting keypresses you will
>>>> likely also have an acpi_video (or perhaps a native intel) backlight
>>>> under /sys/class/backlight and I noticed that panasonic-laptop
>>>> uncondirionally registers its backlight so you may very well end
>>>> up with 2 backlight controls under /sys/class/backlight, which
>>>> we generally try to avoid (so that userspace does not have to
>>>> guess which one to use).
>>>>
>>>> Can you do:
>>>> ls /sys/class/backlight
>>>
>>> toughbook:~ # ls -l /sys/class/backlight/
>>> total 0
>>> lrwxrwxrwx 1 root root 0 Jun 17 14:45 intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
>>> lrwxrwxrwx 1 root root 0 Jun 17 14:49 panasonic -> ../../devices/virtual/backlight/panasonic
>>>
>>>> and let me know the output?
>>>>
>>>> Also if there are 2 backlights there then please do:
>>>> cat /sys/class/backlight/<name>/max_brightness
>>>> to find out the range (0-value)
>>>
>>> toughbook:/sys/class/backlight # grep . */max_brightness
>>> intel_backlight/max_brightness:19531
>>> panasonic/max_brightness:255
>>>
>>>> and then try if they both work by doing:
>>>>
>>>> echo $number > /sys/class/backlight/<name>/brightness
>>>>
>>>> with different $number values in the range and see
>>>> if this actually changes the brightness.
>>>
>>> intel_backlight: does not work
>>> panasonic: does work
>>
>> Ok, so that suggests that the ACPI video bus on your
>> device is defunct, so I guess it also does not report
>> key-presses (see above) ?
>
> Yes, it looks like ACPI video driver is totally useless on that machine.
>
>> This will also need some work then because we want to move
>> to there only being 1 (actually working) backlight-class
>> device. Rather then having multiple and let userspace
>> guess which one it needs to use.
>
> Well, the non-working backlight is coming from the i915 driver, but as this is a very old Chipset (i855 GM) I'd rather be happy it works at all instead of complaining ;-)
> (I have another machine of similar age, hp nc6000 with ati graphics, and there is no way getting it to work somewhat reliably at all)
Ah right, you've got a panasonic + a native intel backlight device.
We are going to need a quirk to (eventually also depending on other changes)
disable the broken intel backlight device.
But that won't fix the keys issue, at least not without an extra
quirk just for that.
I wonder if your machine supports the backlight control part of
the ACPI video bus at all. If not that would explain why it is
not reporting brightness keys and that would also give us a way
to solve this without an extra quirk.
And that would actually also avoid the need for a backlight
quirk too.
Can you pass "acpi_backlight=video" on the kernel commandline
and see if a /sys/class/backlight/acpi_video0 device then
shows up. If it does _not_ show up then indeed there is no
ACPI backlight control at all.
In that case please give the attached patches a try on top
of my last series.
The acpi_video patch should fix your brightness keys then and
the extra panasonic-laptop patch should not make any difference
for the available /sys/class/backlight devices on your laptop,
while filtering out the broken panasonic backlight on Kenneth's
device.
Regards,
Hans
>
>>>> While we are at it, Stefan can you do the same please?
>>>
>>> See above.
>>> But hey, this is an i855GM graphics chip, I'm happy if it is still working *at all* (for example I need to avoid the xf86-intel driver and use the modesetting driver instead to get a usable sytstem)
>>>
>>> And I'm totally happy if all I have to do in the future is a
>>>
>>> option video report_key_events=1
>>>
>>> modprobe.conf file ;-)
>>
>> We really don't want people to have to specify module-options just
>> to have things working.
>
> I understand, but then it's my job to get that DMI match to set this parameter into acpi_video.c ;-)
>
>> Stefam, at least for the backlight class-device issue we will need a DMI
>> quirk, so can you run:
>>
>> sudo dmidecode > dmidecode.txt
>>
>> and then attach the output to your next email, or send me a copy
>> privately ?
>
> I'll send it privately as it is pretty big, but I think
>
> DMI_BOARD_VENDOR, "Matsushita Electric Industrial Co.,Ltd."
> DMI_BOARD_NAME, "CF51-1L"
>
> (Similar to the CF51-2L in acpi/sleep.c) will do.
>
> Best regards,
>
> Stefan
Hi,
On 6/20/22 17:21, Kenneth Chan wrote:
> It took quite a while to do a full compile, just to be safe.
<snip backlight stuff, which looks as expected>
> The mute, volume up/down keys are still duplicated by atkbd after
> applying 0005-platform-x86-panasonic-laptop-filter-out-duplicate-v.patch.
Hmm, can you add a couple of:
pr_info("data 0x%02x\n", data);
at the top of the new panasonic_i8042_filter() function
and then check in dmesg what is output for the volume keys.
The patch should filter out those duplicate keys, unless
I got the codes wrong somehow.
Also can you please try the attached 2 patches on top of my
last series, this should hide the broken panasonic backlight
device and otherwise it should make no difference (but maybe
double check the duplicate brightness keys are not back.
Regards,
Hans
Hi Hans,
On 21.06.22 11:26, Hans de Goede wrote:
> Hi,
>
> On 6/20/22 20:10, Stefan Seyfried wrote:
>> Well, the non-working backlight is coming from the i915 driver, but as this is a very old Chipset (i855 GM) I'd rather be happy it works at all instead of complaining ;-)
>> (I have another machine of similar age, hp nc6000 with ati graphics, and there is no way getting it to work somewhat reliably at all)
>
> Ah right, you've got a panasonic + a native intel backlight device.
>
> We are going to need a quirk to (eventually also depending on other changes)
> disable the broken intel backlight device.
>
> But that won't fix the keys issue, at least not without an extra
> quirk just for that.
>
> I wonder if your machine supports the backlight control part of
> the ACPI video bus at all. If not that would explain why it is
> not reporting brightness keys and that would also give us a way
> to solve this without an extra quirk.
>
> And that would actually also avoid the need for a backlight
> quirk too.
>
> Can you pass "acpi_backlight=video" on the kernel commandline
> and see if a /sys/class/backlight/acpi_video0 device then
> shows up. If it does _not_ show up then indeed there is no
> ACPI backlight control at all.
Nothing new shows up, just panasonic and intel_backlight as before.
> In that case please give the attached patches a try on top
> of my last series.
they do not fix the brightness keys for me.
I did not have time to put in some debugging and will be traveling for
the rest of the week, but I'll take the toughbook with me and will try
to debug this later ;-)
> The acpi_video patch should fix your brightness keys then and
apparently it does not :-(
> the extra panasonic-laptop patch should not make any difference
> for the available /sys/class/backlight devices on your laptop,
> while filtering out the broken panasonic backlight on Kenneth's
> device.
Yes, the panasonic backlight does still work on my machine.
Best regards,
Stefan
--
Stefan Seyfried
"For a successful technology, reality must take precedence over
public relations, for nature cannot be fooled." -- Richard Feynman
Hi Hans,
the patched ACPI video module DOES WORK.
I just managed to actually compile the unmodified source code m(
After patching acpi_video.c, compiling and installing it so that it
actually gets used, everything works fine now.
On 21.06.22 12:23, Stefan Seyfried wrote:
> Hi Hans,
>
> On 21.06.22 11:26, Hans de Goede wrote:
>> Hi,
>>
>> On 6/20/22 20:10, Stefan Seyfried wrote:
>>> Well, the non-working backlight is coming from the i915 driver, but
>>> as this is a very old Chipset (i855 GM) I'd rather be happy it works
>>> at all instead of complaining ;-)
>>> (I have another machine of similar age, hp nc6000 with ati graphics,
>>> and there is no way getting it to work somewhat reliably at all)
>>
>> Ah right, you've got a panasonic + a native intel backlight device.
>>
>> We are going to need a quirk to (eventually also depending on other
>> changes)
>> disable the broken intel backlight device.
>>
>> But that won't fix the keys issue, at least not without an extra
>> quirk just for that.
>>
>> I wonder if your machine supports the backlight control part of
>> the ACPI video bus at all. If not that would explain why it is
>> not reporting brightness keys and that would also give us a way
>> to solve this without an extra quirk.
>>
>> And that would actually also avoid the need for a backlight
>> quirk too.
>>
>> Can you pass "acpi_backlight=video" on the kernel commandline
>> and see if a /sys/class/backlight/acpi_video0 device then
>> shows up. If it does _not_ show up then indeed there is no
>> ACPI backlight control at all.
>
> Nothing new shows up, just panasonic and intel_backlight as before.
>
>> In that case please give the attached patches a try on top
>> of my last series.
>
> they do not fix the brightness keys for me.
YES they do fix it. I just need to actually use the patched module :-)
> I did not have time to put in some debugging and will be traveling for
> the rest of the week, but I'll take the toughbook with me and will try
> to debug this later ;-)
>
>> The acpi_video patch should fix your brightness keys then and
>
> apparently it does not :-(
It does.
Thanks
Stefan
--
Stefan Seyfried
"For a successful technology, reality must take precedence over
public relations, for nature cannot be fooled." -- Richard Feynman
Hi,
On 6/21/22 19:54, Stefan Seyfried wrote:
> Hi Hans,
>
> the patched ACPI video module DOES WORK.
>
> I just managed to actually compile the unmodified source code m(
> After patching acpi_video.c, compiling and installing it so that it actually gets used, everything works fine now.
Great, that is good news. Thank you.
So if Kenneth can figure out why the i8042 filter is not working,
then we can hopefully fix this with my original series + the 2 extra
patches. And then this will be fixed without needing any DMI matches
and generic fixes are always better :)
Regards,
Hans
>
> On 21.06.22 12:23, Stefan Seyfried wrote:
>> Hi Hans,
>>
>> On 21.06.22 11:26, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 6/20/22 20:10, Stefan Seyfried wrote:
>>>> Well, the non-working backlight is coming from the i915 driver, but as this is a very old Chipset (i855 GM) I'd rather be happy it works at all instead of complaining ;-)
>>>> (I have another machine of similar age, hp nc6000 with ati graphics, and there is no way getting it to work somewhat reliably at all)
>>>
>>> Ah right, you've got a panasonic + a native intel backlight device.
>>>
>>> We are going to need a quirk to (eventually also depending on other changes)
>>> disable the broken intel backlight device.
>>>
>>> But that won't fix the keys issue, at least not without an extra
>>> quirk just for that.
>>>
>>> I wonder if your machine supports the backlight control part of
>>> the ACPI video bus at all. If not that would explain why it is
>>> not reporting brightness keys and that would also give us a way
>>> to solve this without an extra quirk.
>>>
>>> And that would actually also avoid the need for a backlight
>>> quirk too.
>>>
>>> Can you pass "acpi_backlight=video" on the kernel commandline
>>> and see if a /sys/class/backlight/acpi_video0 device then
>>> shows up. If it does _not_ show up then indeed there is no
>>> ACPI backlight control at all.
>>
>> Nothing new shows up, just panasonic and intel_backlight as before.
>>
>>> In that case please give the attached patches a try on top
>>> of my last series.
>>
>> they do not fix the brightness keys for me.
>
> YES they do fix it. I just need to actually use the patched module :-)
>
>> I did not have time to put in some debugging and will be traveling for the rest of the week, but I'll take the toughbook with me and will try to debug this later ;-)
>>
>>> The acpi_video patch should fix your brightness keys then and
>>
>> apparently it does not :-(
> It does.
>
> Thanks
> Stefan
Hi Hans,
On Tue, 21 Jun 2022 at 17:34, Hans de Goede <[email protected]> wrote:
>
>
> > The mute, volume up/down keys are still duplicated by atkbd after
> > applying 0005-platform-x86-panasonic-laptop-filter-out-duplicate-v.patch.
>
> Hmm, can you add a couple of:
>
> pr_info("data 0x%02x\n", data);
>
> at the top of the new panasonic_i8042_filter() function
> and then check in dmesg what is output for the volume keys.
>
Volume Down 0xe0 0x2e / 0xe0 0xae
Mute 0xe0 0x20 / 0xe0 0xa0
Volume Up 0xe0 0x30 / 0xe0 0xb0
I replaced those values with these and it filters out the duplicate keys. Yay!!!
>
> The patch should filter out those duplicate keys, unless
> I got the codes wrong somehow.
>
> Also can you please try the attached 2 patches on top of my
> last series, this should hide the broken panasonic backlight
> device and otherwise it should make no difference (but maybe
> double check the duplicate brightness keys are not back.
>
The last 2 patches crash as soon as the panasonic-laptop module is
loaded. It's compiled against kernel v5.18.5. Please see the
attachment. I'm going to compile it against the latest and see if it
works.
--
Kenneth
Hi,
On 6/24/22 07:14, Kenneth Chan wrote:
> Hi Hans,
>
>
> On Tue, 21 Jun 2022 at 17:34, Hans de Goede <[email protected]> wrote:
>>
>>
>>> The mute, volume up/down keys are still duplicated by atkbd after
>>> applying 0005-platform-x86-panasonic-laptop-filter-out-duplicate-v.patch.
>>
>> Hmm, can you add a couple of:
>>
>> pr_info("data 0x%02x\n", data);
>>
>> at the top of the new panasonic_i8042_filter() function
>> and then check in dmesg what is output for the volume keys.
>>
>
> Volume Down 0xe0 0x2e / 0xe0 0xae
> Mute 0xe0 0x20 / 0xe0 0xa0
> Volume Up 0xe0 0x30 / 0xe0 0xb0
>
> I replaced those values with these and it filters out the duplicate keys. Yay!!!
That is great.
>> The patch should filter out those duplicate keys, unless
>> I got the codes wrong somehow.
>>
>> Also can you please try the attached 2 patches on top of my
>> last series, this should hide the broken panasonic backlight
>> device and otherwise it should make no difference (but maybe
>> double check the duplicate brightness keys are not back.
>>
>
> The last 2 patches crash as soon as the panasonic-laptop module is
> loaded. It's compiled against kernel v5.18.5. Please see the
> attachment. I'm going to compile it against the latest and see if it
> works.
No need to compile against the latest, I messed things up, sorry.
To fix the crash the following diff is necessary:
diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index 0fa7695089e2..b8fa0a64698b 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -1011,10 +1011,10 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
result = PTR_ERR(pcc->backlight);
goto out_input;
}
- }
- /* read the initial brightness setting from the hardware */
- pcc->backlight->props.brightness = pcc->sinf[SINF_AC_CUR_BRIGHT];
+ /* read the initial brightness setting from the hardware */
+ pcc->backlight->props.brightness = pcc->sinf[SINF_AC_CUR_BRIGHT];
+ }
/* Reset initial sticky key mode since the hardware register state is not consistent */
acpi_pcc_write_sset(pcc, SINF_STICKY_KEY, 0);
I'm going to send out a whole new version of my entire series, including
an updated i8042 filter. Please drop all my previous patches and try
the new version (I'll put on you in the Cc of the upstream submission of
the series).
Regards,
Hans