Date:   Tue, 21 Jun 2022 11:26:51 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Stefan Seyfried <stefan.seyfried@...glemail.com>,
        Kenneth Chan <kenneth.t.chan@...il.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Stefan Seyfried <seife+kernel@...systems.com>
Subject: Re: [PATCH 2/2] platform/x86: panasonic-laptop: allow to use all
 hotkeys

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
View attachment "0001-platform-x86-panasonic-laptop-Use-acpi_video_get_bac.patch" of type "text/x-patch" (1938 bytes)

View attachment "0002-ACPI-video-Change-how-we-determine-if-brightness-key.patch" of type "text/x-patch" (2465 bytes)

Powered by blists - more mailing lists