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

[Stefan Seyfried]

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