Bug 204077

Created attachment 283563 [details]
dmesg

Created attachment 283565 [details]
evemu-record

Only /dev/input/event3:	Video Bus seems to generate events. Keyboard don't send any event on brightness up/down key

Created attachment 283567 [details]
lsmod

After adding both "video.brightness_switch_enabled=0" and "video.report_key_events=1" to the kernel commandline brightness is changed only once but no event is send on video bus.

With just "video.brightness_switch_enabled=0" brightness is changed only one and brightness up/down events are send on video bus.

For both acpi_backlight=vendor and acpi_backlight=native there is nothing under /sys/class/backlight, brightness change only once and video bus sends brightness up/down key.

Thank you for the bug-report.

I've tested that just calling backlight_force_update() and not sending any keypresses to userspace still triggers the brightness change OSD popup. At least with GNOME3 this works as expected.

The way I've tested this is by setting video.report_key_events=1 on a machine which uses the acpi_video0 backlight interface. This means no key-events are reported to userspace. Since userspace does not react and since video.brightness_switch_enabled is its default value of 1, this causes the kernel to change the brightness itself and then call backlight_force_update(), which does properly trigger the OSD. So it seems that the fix for your machine would be to make acpi_video_device_notify() call backlight_force_update() and not do anything else (on your machine).

I've prepared a patch for this, which I will attach here.

Please build a kernel with this patch (see your distros documentation) and let me know if this fixes things.

After booting the new kernel, please do:

cat /sys/module/video/parameters/hw_brightness_change

And verify it is "1", if it is not then the DMI match in the patch is somehow wrong.

If the DMI match is wrong (should not happen) please try to test the rest of the patch by passing video.hw_brightness_change=1 on the kernel commandline. Note you only need to do this if the DMI match does not work, please try without this first.

If "cat /sys/module/video/parameters/hw_brightness_change" returns "1" then you should be able to use the hotkeys to change the brightness, get a change of only 1 level/step and get the ODS (at least with GNOME3).

Created attachment 283623 [details]
[PATCH] ACPI / video: Add new hw_brightness_change quirk, set it on PB Easynote MZ35

When you can confirm this patch fixes things I will submit it upstream, is it ok if I add the unredacted version of:

Reported-and-tested-by: Kacper <cosiekvfj@redacted>

to the commit message when submitting it upstream?

> Please build a kernel with this patch (see your distros documentation)

It was actually easier than I thought. I just downloaded repo for 5.1 kernel from my distro, edited PKGBUILD to add patch command after all the other patches, installed some deps and voilà. Tomorrow morning I had nice .pkg.tar.gz file waiting for me.


> cat /sys/module/video/parameters/hw_brightness_change

I will assume that you meant hw_changes_brightness. It is indeed 1.

> by passing video.hw_brightness_change=1 on the kernel commandline

It's easier to just "sudo tee /sys/module/video/parameters/hw_brightness_change"
You can change any parameter in "/sys/module/video/parameters/" that way while system is on. No reboot required. :)

> If "cat /sys/module/video/parameters/hw_brightness_change" returns "1" then
> you should be able to use the hotkeys to change the brightness, get a change
> of only 1 level/step and get the ODS (at least with GNOME3).

I can confirm that this patch allow me to change brightness by only 1 level/step. evemu-record don't show any events from "video bus" while changing brightness. backlight_force_update() is working differently than just sending events thru "video bus"?

I didn't test ODS.
(I just didn't install any userspace brightness changer because I already had 2, laptop itself and kernel module. Is there a easy way to check if backlight_force_update() is called?)

Also when I set hw_changes_brightness to 0 everything seems to work like before.  2 brightness steps per one brightness key and key events on "video bus"


> When you can confirm this patch fixes things I will submit it upstream, is it
> ok if I add the unredacted version of:

> Reported-and-tested-by: Kacper <cosiekvfj@redacted>

> to the commit message when submitting it upstream?

Yes. Please include my full name "Kacper Piwiński"

Do this kind of hardware workaround fixes really need to be that deep i kernel modules? I thought it would be just some quirk database higher up. :)

> Some machines change the brightness themselves when a brightness hotkey
gets pressed, despite us telling them not to.

Do you also want to test different acpi_osi= kernel parameters?


> Subject: [PATCH] ACPI / video: Add new hw_brightness_change quirk, set it on…
> This commit adds a new hw_brightness_change quirk which makes…

But you actually named it hw_changes_brightness :)

(In reply to Kacper from comment #9)
> > cat /sys/module/video/parameters/hw_brightness_change
> 
> I will assume that you meant hw_changes_brightness. It is indeed 1.

Yeah my bad somehow between writing the code and writing the commit message (and the bugzilla comment) I ended up changing the quirk name in my head.

> > Subject: [PATCH] ACPI / video: Add new hw_brightness_change quirk, set it
> on…
> > This commit adds a new hw_brightness_change quirk which makes…
> 
> But you actually named it hw_changes_brightness :)

Thank you for catching that, I've fixed this before submitting it upstream.

> It's easier to just "sudo tee
> /sys/module/video/parameters/hw_brightness_change"
> You can change any parameter in "/sys/module/video/parameters/" that way
> while system is on. No reboot required. :)

Right this parameter can also be changed runtime, that is not always the case, so I always tell people to put it on the kernel commandline.

> I can confirm that this patch allow me to change brightness by only 1
> level/step. evemu-record don't show any events from "video bus" while
> changing brightness. backlight_force_update() is working differently than
> just sending events thru "video bus"?

The "video bus" device behaves as a human input device (kbd/mouse) so it sends keypresses through a /dev/input/event# node. The backlight_force_update() call sends a udev change event from /sys/class/backlight/acpi_video0/uevent. 

Under GNOME3 /usr/libexec/gsd-power, which controls the display brightness listens to this (through libudev and udevd), and then activates the OSD on the change event.

> I didn't test ODS.
> (I just didn't install any userspace brightness changer because I already
> had 2, laptop itself and kernel module. Is there a easy way to check if
> backlight_force_update() is called?)

No not really, but since you're no longer seeing the keypresses I'm pretty sure that everything is working as it should, so that is fine.

> Also when I set hw_changes_brightness to 0 everything seems to work like
> before.  2 brightness steps per one brightness key and key events on "video
> bus"

Right, that is as expected, the default value is -1 which means auto / use DMI quirks, if you force it to 0 you get what you ask for :)

Note on the MZ35 you do not see the -1 since the dmi quirk overrides it to 1:

+static int video_hw_changes_brightness(
+       const struct dmi_system_id *d)
+{
+       if (hw_changes_brightness == -1)
+               hw_changes_brightness = 1;
+       return 0;
+}
+

If however you set it to 0 on the kernel commandline then the DMI quirk will not touch it and the same goes for runtime changes since the DMI quirk code runs only once during boot.

> > When you can confirm this patch fixes things I will submit it upstream, is
> it
> > ok if I add the unredacted version of:
> 
> > Reported-and-tested-by: Kacper <cosiekvfj@redacted>
> 
> > to the commit message when submitting it upstream?
> 
> Yes. Please include my full name "Kacper Piwiński"

Ok will do.

(In reply to Kacper from comment #10)
> Do this kind of hardware workaround fixes really need to be that deep i
> kernel modules? I thought it would be just some quirk database higher up. :)

Sometimes we can fix things with e.g. keymapping changes in systemd's hwdb, but in this case we need to change how the acpi_video_device_notify() function works, which we can only do at the kernel level.

> > Some machines change the brightness themselves when a brightness hotkey
> gets pressed, despite us telling them not to.
> 
> Do you also want to test different acpi_osi= kernel parameters?

No that is not necessary.

I have two more questions.

> * Some machines change the brightness themselves when a brightness
	 * hotkey gets pressed, despite us telling them not to.

Where do we tell them to not do that? :)


I wonder if It was possible to get the same behaviour by setting "brightness_switch_enabled" and "report_key_events" instead of creating another module parameter? :)


I will report if everything is correct when I will test 5.3 kernel :)

(In reply to Kacper from comment #12)
> I have two more questions.
> 
> > * Some machines change the brightness themselves when a brightness
>        * hotkey gets pressed, despite us telling them not to.
> 
> Where do we tell them to not do that? :)

See acpi_video_bus_DOS and its callers in drivers/acpi/acpi_video.c 

> I wonder if It was possible to get the same behaviour by setting
> "brightness_switch_enabled" and "report_key_events" instead of creating
> another module parameter? :)

No that is not possible.

Everything seems to work ok. Now I can set any of my 8 levels of brightness.

As I understand, setting
"brightness_switch_enabled" to 0 and "report_key_events" to 1
won't produce the same behavior because it wouldn't call "backlight_force_update()" ?

(In reply to Kacper from comment #14)
> Everything seems to work ok. Now I can set any of my 8 levels of brightness.

Good, I believe that this bug can be closed then.

> As I understand, setting
> "brightness_switch_enabled" to 0 and "report_key_events" to 1
> won't produce the same behavior because it wouldn't call
> "backlight_force_update()" ?

Right, so that would work, but you would not get the OSD (on screen display) notification about the brightness changing.

> > > * Some machines change the brightness themselves when a brightness
> > > * hotkey gets pressed, despite us telling them not to.
> >
> > Where do we tell them to not do that? :)
>
> See acpi_video_bus_DOS and its callers in drivers/acpi/acpi_video.c



static int acpi_video_bus_start_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0,
				  acpi_osi_is_win8() ? 1 : 0);
}`

But on my machine acpi_osi_is_win8() return false (I build simple module to test that) so actually we never tell the BIOS to not change brightness by itself.

Sorry for double comment. Missclick

(In reply to Kacper from comment #17)
> But on my machine acpi_osi_is_win8() return false (I build simple module to
> test that) so actually we never tell the BIOS to not change brightness by
> itself.

Older BIOS-es are simply expected to not change the brightness by themselves without needing the lcd_flag to be 1. I guess it could be that your BIOS is
somehow somewhat in between the behavior expected from older BIOS-es vs that expected from newer BIOS-es.

If you want you can try dropping the acpi_osi_is_win8() check and always pass 1 for the lcd_flag value passed to acpi_video_bus_DOS() combined with adding
video.hw_changes_brightness=0 on the kernel commandline to disable my patch. I guess this might do the trick too.

But then we would need to have a DMI based quirk to override the lcd_flag value... So I do not think that would be any better.

(In reply to Hans de Goede)

First, thank you for your help.

I have build kernel with:

static int acpi_video_bus_start_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0, 1);
}

static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0, 0);
}

But there was no change in brightness key behavior. BIOS was still controlling the brightness keys.



Couple more points:


1.

 *	lcd_flag	:
 *		0.	The system BIOS should automatically control the brightness level
 *			of the LCD when the power changes from AC to DC
 *		1.	The system BIOS should NOT automatically control the brightness
 *			level of the LCD when the power changes from AC to DC.

This description of lcd_flag talk only about BIOS changing brighness when you plug your charger. But we where talking about it like it was controlling if BIOS should handle brightness keys.

2.

static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0,
				  acpi_osi_is_win8() ? 0 : 1);
}

As I understand this get called when the driver no longer use that hardware. Why we set lcd_flag to 1 then? If we don't control brightness keys then we should tell hardware to control it?

3.
So maybe it should look like 

static int acpi_video_bus_start_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0,
				  acpi_osi_is_win8() ? 1 : 0);
}

static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0, 0);
}

if we are worrying that setting it to 1 on non win8 hardware will expose some bugs. But that seems to me unlikely considering that we already was setting it to 1 on device stopping.

4.
So maybe it should always set it like this?

static int acpi_video_bus_start_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0, 1);
}

static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
{
	return acpi_video_bus_DOS(video, 0, 0);
}

regardless if machine is win8+. As I understand we always wants to control brightness keys ourselves. So we always should set lcd_flag to 1 and only with buggy hardware(like mine) we should let hardware to control those keys.

5.
Why are we not using bios_flag 3?

 *		3.	The system BIOS should not automatically switch (toggle) the
 *			active display output, but instead generate the display switch
 *			event notify code.

instead we set it to 0.

6.
typo:

		/*
		 * if the function of acpi_video_register is already called,
		 * don't register the acpi_vide_bus again and return no error.
		 */

acpi_vide_bus instead of acpi_video_bus

;)



Cheers, let me know if any of this makes any sense and if I could write some patches for it.

(In reply to Kacper from comment #19)
> (In reply to Hans de Goede)
> 
> First, thank you for your help.
> 
> I have build kernel with:
> 
> static int acpi_video_bus_start_devices(struct acpi_video_bus *video)
> {
>       return acpi_video_bus_DOS(video, 0, 1);
> }
> 
> static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
> {
>       return acpi_video_bus_DOS(video, 0, 0);
> }
> 
> But there was no change in brightness key behavior. BIOS was still
> controlling the brightness keys.

Right, so this shows that the quirk which my patch (which has been merged in the mean time) for this adds is really necessary.

> Couple more points:
> 
> 
> 1.
> 
>  *    lcd_flag        :
>  *            0.      The system BIOS should automatically control the
>  brightness level
>  *                    of the LCD when the power changes from AC to DC
>  *            1.      The system BIOS should NOT automatically control the
>  brightness
>  *                    level of the LCD when the power changes from AC to DC.
> 
> This description of lcd_flag talk only about BIOS changing brighness when
> you plug your charger. But we where talking about it like it was controlling
> if BIOS should handle brightness keys.

As the comment above acpi_video_bus_start_devices() explains this has changed with windows 8 and later, firmwares targetting win 8 and later will automatically adjust the brightness on hotkey presses unless the lcd_flag is set to 1.

> 2.
> 
> static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
> {
>       return acpi_video_bus_DOS(video, 0,
>                                 acpi_osi_is_win8() ? 0 : 1);
> }
> 
> As I understand this get called when the driver no longer use that hardware.
> Why we set lcd_flag to 1 then? If we don't control brightness keys then we
> should tell hardware to control it?

Notice that we do set it to 0 for win8+ systems, we only set it to 1 for
older systems. We already did that before the changes were made to behave differently on win8+ systems:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=efaa14c7e981b

As for wht we pass 1 on older systems, that pre-dates git history it seems.

Also note that this code only runs though if someone does a "rmmod i915" and/or "rmmod video". Which normally never happens.

> 3.
> So maybe it should look like 
> 
> static int acpi_video_bus_start_devices(struct acpi_video_bus *video)
> {
>       return acpi_video_bus_DOS(video, 0,
>                                 acpi_osi_is_win8() ? 1 : 0);
> }
> 
> static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
> {
>       return acpi_video_bus_DOS(video, 0, 0);
> }
> 
> if we are worrying that setting it to 1 on non win8 hardware will expose
> some bugs. But that seems to me unlikely considering that we already was
> setting it to 1 on device stopping.

Maybe, as said I do not know why the 1 on older systems is there and I see no reason to change this at this moment.

> 4.
> So maybe it should always set it like this?
> 
> static int acpi_video_bus_start_devices(struct acpi_video_bus *video)
> {
>       return acpi_video_bus_DOS(video, 0, 1);
> }
> 
> static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
> {
>       return acpi_video_bus_DOS(video, 0, 0);
> }
> 
> regardless if machine is win8+. As I understand we always wants to control
> brightness keys ourselves. So we always should set lcd_flag to 1 and only
> with buggy hardware(like mine) we should let hardware to control those keys.

A lot of this is magic which we have collected over the years as we learn how to make things work with each generation of hw. I'm pretty sure the changes you suggest will likely break some XP area hw. I believe this is a case of if it is not broken do not fix it. We simply do not have access to all hardware, esp. not to older hw which still has users out there. So unless there is a good reason to do so this is best left unchanged IMHO.

> 5.
> Why are we not using bios_flag 3?
> 
>  *            3.      The system BIOS should not automatically switch
>  (toggle) the
>  *                    active display output, but instead generate the display
>  switch
>  *                    event notify code.
> 
> instead we set it to 0.

I guess Windows has always set this to 0, so we do to and as I'm not aware of any issues with the BIOS changing the output underneath us I think out current behavior is fine,

> 
> 6.
> typo:
> 
>               /*
>                * if the function of acpi_video_register is already called,
>                * don't register the acpi_vide_bus again and return no error.
>                */
> 
> acpi_vide_bus instead of acpi_video_bus

Feel free to submit a patch upstream to fix this :)