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

[Hans de Goede]

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,
> 
>     StefanFrom f9afd65564e1f4e8e41b0cdbc754b11fc0e2edf6 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Tue, 21 Jun 2022 11:20:42 +0200
Subject: [PATCH 1/2] platform/x86: panasonic-laptop: Use
 acpi_video_get_backlight_type()

Use acpi_video_get_backlight_type() to determine if we should register
the panasonic specific backlight interface. To avoid registering this
on systems where the ACPI or GPU native backlight control methods
should be used instead.

Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
 drivers/platform/x86/panasonic-laptop.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index a4c82b3a81cf..0fa7695089e2 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -998,15 +998,19 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
 		pr_err("Couldn't retrieve BIOS data\n");
 		goto out_input;
 	}
-	/* initialize backlight */
-	memset(&props, 0, sizeof(struct backlight_properties));
-	props.type = BACKLIGHT_PLATFORM;
-	props.max_brightness = pcc->sinf[SINF_AC_MAX_BRIGHT];
-	pcc->backlight = backlight_device_register("panasonic", NULL, pcc,
-						   &pcc_backlight_ops, &props);
-	if (IS_ERR(pcc->backlight)) {
-		result = PTR_ERR(pcc->backlight);
-		goto out_input;
+
+	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+		/* initialize backlight */
+		memset(&props, 0, sizeof(struct backlight_properties));
+		props.type = BACKLIGHT_PLATFORM;
+		props.max_brightness = pcc->sinf[SINF_AC_MAX_BRIGHT];
+		
+		pcc->backlight = backlight_device_register("panasonic", NULL, pcc,
+							   &pcc_backlight_ops, &props);
+		if (IS_ERR(pcc->backlight)) {
+			result = PTR_ERR(pcc->backlight);
+			goto out_input;
+		}
 	}
 
 	/* read the initial brightness setting from the hardware */
-- 
2.36.0

From 70d07e9157bb2813726986d068fb0c99e1a619d9 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Tue, 21 Jun 2022 11:12:51 +0200
Subject: [PATCH 2/2] ACPI: video: Change how we determine if brightness
 key-presses are handled

Some systems have an ACPI video bus but not ACPI video devices with
backlight capability. On these devices brightness key-presses are
(logically) not reported through the ACPI video bus.

Change how acpi_video_handles_brightness_key_presses() determines if
brightness key-presses are handled by the ACPI video driver to avoid
vendor specific drivers/platform/x86 drivers filtering out their
brightness key-presses even though they are the only ones reporting
these presses.

Reported-by: Stefan Seyfried <seife+kernel@xxxxxxxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
 drivers/acpi/acpi_video.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 01c3f62295c3..be7f4d1912de 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -79,6 +79,7 @@ module_param(device_id_scheme, bool, 0444);
 static int only_lcd = -1;
 module_param(only_lcd, int, 0444);
 
+static bool has_backlight;
 static int register_count;
 static DEFINE_MUTEX(register_count_mutex);
 static DEFINE_MUTEX(video_list_lock);
@@ -1230,6 +1231,9 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	acpi_video_device_bind(video, data);
 	acpi_video_device_find_cap(data);
 
+	if (data->cap._BCM && data->cap._BCL)
+		has_backlight = true;
+
 	mutex_lock(&video->device_list_lock);
 	list_add_tail(&data->entry, &video->video_device_list);
 	mutex_unlock(&video->device_list_lock);
@@ -2276,6 +2280,7 @@ void acpi_video_unregister(void)
 		cancel_delayed_work_sync(&video_bus_register_backlight_work);
 		acpi_bus_unregister_driver(&acpi_video_bus);
 		register_count = 0;
+		has_backlight = false;
 	}
 	mutex_unlock(&register_count_mutex);
 }
@@ -2294,13 +2299,7 @@ EXPORT_SYMBOL(acpi_video_register_backlight);
 
 bool acpi_video_handles_brightness_key_presses(void)
 {
-	bool have_video_busses;
-
-	mutex_lock(&video_list_lock);
-	have_video_busses = !list_empty(&video_bus_head);
-	mutex_unlock(&video_list_lock);
-
-	return have_video_busses &&
+	return has_backlight &&
 	       (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
 }
 EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
-- 
2.36.0