amdgpu, dc, backlight brightness

amdgpu, dc, backlight brightness

[Andriy Gapon]

I see that amdgpu_atif_handler() has this comment and code:
                /* todo: add DC handling */
                if ((req.pending & ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST) &&
                    !amdgpu_device_has_dc_support(adev)) {
...
                }

So, this makes me curious how a typical Linux distribution handles backlight
brightness change via special keys hooked to ACPI.

This is what I see on FreeBSD.
- a special key is pressed
- there is a bunch of uninteresting ACPI and kernel stuff involving EC
- ACPI notification 0x86 on VGA.LCD device is handled by acpi_video driver
- the driver invokes VGA.LCD._BCM method
- if I read the ASL of my system correctly, ACPI does not touch any hardware
  but simply saves some things like the requested brightness level
- then ACPI posts notification 0x81 on VGA device
- the notification gets routed to amdgpu
- amdgpu invokes VGA.ATIF and gets some interesting data from ACPI
- and this is where things stop in the DC case (because of the code above)

In the non-DC case amdgpu would actually set the brightness based on the data
returned from ATIF.  radeon driver also did the same as far as I can see.

So, how things work in the DC case?

I see that Linux acpi_video does something that FreeBSD doesn't do, it posts
KEY_BRIGHTNESSUP / KEY_BRIGHTNESSDOWN / etc.  I guess that in the end this is
similar to just having the corresponding multimedia keys on a keyboard.
Is this how the brightness control supposed to work?
So, there must be some userland program listening for those keys and somehow
knowing about amdgpu backlight controls (e.g., /sys/class/backlight/amdgpu_bl0).
Is that correct?

I tried a live image of Void Linux (LXQT flavor).  While it does handle the
special brightness keys it seems to do it without actually controlling the
backlight.  That is, it makes the picture lighter / darker, but values under
/sys/class/backlight/amdgpu_bl0 do not change.  If I set brightness under
/sys/class/backlight/amdgpu_bl0 to some low level then the brightness up key
cannot make the screen brighter.  So, it appears to be just a rendering /
composition trick.

Thank you.
-- 
Andriy Gapon
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amdgpu, dc, backlight brightness

[Alex Deucher]

On Wed, Apr 29, 2020 at 3:29 AM Andriy Gapon <avg@freebsd.org> wrote:
>
>
> I see that amdgpu_atif_handler() has this comment and code:
>                 /* todo: add DC handling */
>                 if ((req.pending & ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST) &&
>                     !amdgpu_device_has_dc_support(adev)) {
> ...
>                 }
>
> So, this makes me curious how a typical Linux distribution handles backlight
> brightness change via special keys hooked to ACPI.
>
> This is what I see on FreeBSD.
> - a special key is pressed
> - there is a bunch of uninteresting ACPI and kernel stuff involving EC
> - ACPI notification 0x86 on VGA.LCD device is handled by acpi_video driver
> - the driver invokes VGA.LCD._BCM method
> - if I read the ASL of my system correctly, ACPI does not touch any hardware
>   but simply saves some things like the requested brightness level
> - then ACPI posts notification 0x81 on VGA device
> - the notification gets routed to amdgpu
> - amdgpu invokes VGA.ATIF and gets some interesting data from ACPI
> - and this is where things stop in the DC case (because of the code above)
>
> In the non-DC case amdgpu would actually set the brightness based on the data
> returned from ATIF.  radeon driver also did the same as far as I can see.
>
> So, how things work in the DC case?
>
> I see that Linux acpi_video does something that FreeBSD doesn't do, it posts
> KEY_BRIGHTNESSUP / KEY_BRIGHTNESSDOWN / etc.  I guess that in the end this is
> similar to just having the corresponding multimedia keys on a keyboard.
> Is this how the brightness control supposed to work?
> So, there must be some userland program listening for those keys and somehow
> knowing about amdgpu backlight controls (e.g., /sys/class/backlight/amdgpu_bl0).
> Is that correct?
>
> I tried a live image of Void Linux (LXQT flavor).  While it does handle the
> special brightness keys it seems to do it without actually controlling the
> backlight.  That is, it makes the picture lighter / darker, but values under
> /sys/class/backlight/amdgpu_bl0 do not change.  If I set brightness under
> /sys/class/backlight/amdgpu_bl0 to some low level then the brightness up key
> cannot make the screen brighter.  So, it appears to be just a rendering /
> composition trick.

I think it varies from OEM to OEM and whatever windows required at the
time.  The sbios may also do different things depending on the osi
string passed to ACPI.  Originally, ACPI handled it all directly.
Then we got some laptops which which did the whole event via ATIF
thing (even that I think varied based on the .  Later, I think the
keys just produced brightness events and it was up to the OS to do
something with them so the user's environment would catch the events
and adjust the backlight via the standard OS backlight control
interface.  We never hooked up the ATIF stuff to DC since I don't
recall ever running into any laptops that used it for backlight
control (the code was carried over from radeon when we forked amdgpu).

Alex
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amdgpu, dc, backlight brightness

[Andriy Gapon]

On 29/04/2020 21:15, Alex Deucher wrote:
> I think it varies from OEM to OEM and whatever windows required at the
> time.  The sbios may also do different things depending on the osi
> string passed to ACPI.  Originally, ACPI handled it all directly.
> Then we got some laptops which which did the whole event via ATIF
> thing (even that I think varied based on the .  Later, I think the
> keys just produced brightness events and it was up to the OS to do
> something with them so the user's environment would catch the events
> and adjust the backlight via the standard OS backlight control
> interface.  We never hooked up the ATIF stuff to DC since I don't
> recall ever running into any laptops that used it for backlight
> control (the code was carried over from radeon when we forked amdgpu).

Thank you for the information!
Indeed, I see that there are so many quirks in how brightness keys are handled
by firmware.  Ranging from actually changing the brightness to posting ACPI
events to posting key codes.  And then those options are not exclusive of each
other.

Just in case, I've written a bit of code for ATIF handler to control the
backlight in the DC case.  I doubt that it is very useful, it was mostly an
exercise for myself.

----------------------------------------------------------------------------
commit ed2ca1d7e3fbdb641d9a1bc2de9b88e2927ff1bd
Author: Andriy Gapon <avg@FreeBSD.org>
Date:   Thu Apr 30 14:47:11 2020 +0300

    amdgpu_acpi: perform automatic backlight adjustment in the DC case too

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 04bbd8f41441c..62fbae1177091 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -444,7 +444,6 @@ static int amdgpu_atif_handler(struct amdgpu_device *adev,

 		DRM_DEBUG_DRIVER("ATIF: %d pending SBIOS requests\n", count);

-		/* todo: add DC handling */
 		if ((req.pending & ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST) &&
 		    !amdgpu_device_has_dc_support(adev)) {
 			struct amdgpu_encoder *enc = atif->encoder_for_bl;
@@ -463,6 +462,34 @@ static int amdgpu_atif_handler(struct amdgpu_device *adev,
 #endif
 			}
 		}
+#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
+		if ((req.pending & ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST) &&
+		    amdgpu_device_has_dc_support(adev)) {
+			struct amdgpu_display_manager *dm = &adev->dm;
+			struct backlight_device *bd = dm->backlight_dev;
+
+			if (bd) {
+				DRM_DEBUG_DRIVER("Changing brightness to %d\n",
+						 req.backlight_level);
+
+				/*
+				 * Newer Linux has
+				 * backlight_device_set_brightness, but it is
+				 * hardwired to post BACKLIGHT_UPDATE_SYSFS.
+				 */
+				mutex_lock(&bd->ops_lock);
+				if (bd->ops &&
+				    req.backlight_level <= bd->props.max_brightness) {
+					bd->props.brightness = req.backlight_level;
+					backlight_update_status(bd);
+				}
+				mutex_unlock(&bd->ops_lock);
+#if 0
+				backlight_generate_event(bd, BACKLIGHT_UPDATE_HOTKEY);
+#endif
+			}
+		}
+#endif
 		if (req.pending & ATIF_DGPU_DISPLAY_EVENT) {
 			if ((adev->flags & AMD_IS_PX) &&
 			    amdgpu_atpx_dgpu_req_power_for_displays()) {



-- 
Andriy Gapon
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: amdgpu, dc, backlight brightness

[Alex Deucher]

On Mon, May 4, 2020 at 4:02 AM Andriy Gapon <avg@freebsd.org> wrote:
>
> On 29/04/2020 21:15, Alex Deucher wrote:
> > I think it varies from OEM to OEM and whatever windows required at the
> > time.  The sbios may also do different things depending on the osi
> > string passed to ACPI.  Originally, ACPI handled it all directly.
> > Then we got some laptops which which did the whole event via ATIF
> > thing (even that I think varied based on the .  Later, I think the
> > keys just produced brightness events and it was up to the OS to do
> > something with them so the user's environment would catch the events
> > and adjust the backlight via the standard OS backlight control
> > interface.  We never hooked up the ATIF stuff to DC since I don't
> > recall ever running into any laptops that used it for backlight
> > control (the code was carried over from radeon when we forked amdgpu).
>
> Thank you for the information!
> Indeed, I see that there are so many quirks in how brightness keys are handled
> by firmware.  Ranging from actually changing the brightness to posting ACPI
> events to posting key codes.  And then those options are not exclusive of each
> other.
>
> Just in case, I've written a bit of code for ATIF handler to control the
> backlight in the DC case.  I doubt that it is very useful, it was mostly an
> exercise for myself.
>

Care to send it out as a proper patch?  I don't mind applying it on
the off chance there are some laptops out there that might use this
path.

Alex

> ----------------------------------------------------------------------------
> commit ed2ca1d7e3fbdb641d9a1bc2de9b88e2927ff1bd
> Author: Andriy Gapon <avg@FreeBSD.org>
> Date:   Thu Apr 30 14:47:11 2020 +0300
>
>     amdgpu_acpi: perform automatic backlight adjustment in the DC case too
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 04bbd8f41441c..62fbae1177091 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -444,7 +444,6 @@ static int amdgpu_atif_handler(struct amdgpu_device *adev,
>
>                 DRM_DEBUG_DRIVER("ATIF: %d pending SBIOS requests\n", count);
>
> -               /* todo: add DC handling */
>                 if ((req.pending & ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST) &&
>                     !amdgpu_device_has_dc_support(adev)) {
>                         struct amdgpu_encoder *enc = atif->encoder_for_bl;
> @@ -463,6 +462,34 @@ static int amdgpu_atif_handler(struct amdgpu_device *adev,
>  #endif
>                         }
>                 }
> +#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
> defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
> +               if ((req.pending & ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST) &&
> +                   amdgpu_device_has_dc_support(adev)) {
> +                       struct amdgpu_display_manager *dm = &adev->dm;
> +                       struct backlight_device *bd = dm->backlight_dev;
> +
> +                       if (bd) {
> +                               DRM_DEBUG_DRIVER("Changing brightness to %d\n",
> +                                                req.backlight_level);
> +
> +                               /*
> +                                * Newer Linux has
> +                                * backlight_device_set_brightness, but it is
> +                                * hardwired to post BACKLIGHT_UPDATE_SYSFS.
> +                                */
> +                               mutex_lock(&bd->ops_lock);
> +                               if (bd->ops &&
> +                                   req.backlight_level <= bd->props.max_brightness) {
> +                                       bd->props.brightness = req.backlight_level;
> +                                       backlight_update_status(bd);
> +                               }
> +                               mutex_unlock(&bd->ops_lock);
> +#if 0
> +                               backlight_generate_event(bd, BACKLIGHT_UPDATE_HOTKEY);
> +#endif
> +                       }
> +               }
> +#endif
>                 if (req.pending & ATIF_DGPU_DISPLAY_EVENT) {
>                         if ((adev->flags & AMD_IS_PX) &&
>                             amdgpu_atpx_dgpu_req_power_for_displays()) {
>
>
>
> --
> Andriy Gapon
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx