Re: [PATCH 2/2] platform/x86: panasonic-laptop: allow to use all hotkeys

[Stefan Seyfried <stefan.seyfried@googlemail.com>]

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