Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44642 )
Change subject: soc/amd/common: add rudimentary ATIF support ......................................................................
soc/amd/common: add rudimentary ATIF support
The Linux kenerl drive for AMD gpu currently has a floor value of 12 for brightness settings (AMDGPU_DM_DEFAULT_MIN_BACKLIGHT). AMD indicates they did this because they were concerned with certain panels flickering at lower backlight values. However, for unaffected panels it's desirable to be able to have the panel "turn off" at the lowest backlight setting. The only way to do that is to provide ATIF bindings that indicate backlight range.
Option SOC_AMD_COMMON_BLOCK_GRAPHICS_ATIF is added to provide a full range for the backlight setting. If needed, this path can be built upon for fuller support, but for the time being this is the only thing necessary to make the backlight be full range.
BUG=b:163583825
Change-Id: If76801a8daf6a5e56ba7d118956f3ebce74e567a Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/soc/amd/common/block/graphics/Kconfig M src/soc/amd/common/block/graphics/graphics.c 2 files changed, 109 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/44642/1
diff --git a/src/soc/amd/common/block/graphics/Kconfig b/src/soc/amd/common/block/graphics/Kconfig index 8aa2a20..4cda353 100644 --- a/src/soc/amd/common/block/graphics/Kconfig +++ b/src/soc/amd/common/block/graphics/Kconfig @@ -3,3 +3,11 @@ default n help Select this option to use AMD common graphics driver support. + +config SOC_AMD_COMMON_BLOCK_GRAPHICS_ATIF + bool + depends on SOC_AMD_COMMON_BLOCK_GRAPHICS + help + Select this option to provide ATIF method with display brightness querying. + Currently, the exported values only open up 0-255 as the brightness range for + the display. diff --git a/src/soc/amd/common/block/graphics/graphics.c b/src/soc/amd/common/block/graphics/graphics.c index 466a91c..24cabfe 100644 --- a/src/soc/amd/common/block/graphics/graphics.c +++ b/src/soc/amd/common/block/graphics/graphics.c @@ -1,13 +1,114 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> #include <device/pci.h> #include <device/pci_ids.h>
+#define ATIF_FUNCTION_VERIFY_INTERFACE 0x0 +struct atif_verify_interface_output { + uint16_t size; /* Size of this object, including size field */ + uint16_t version; + uint32_t supported_notifications; + uint32_t supported_functions; /* Bit n set if function n+1 supported. */ +}; + +#define ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS 0x10 +# define ATIF_QBTC_REQUEST_LCD1 0 +/* error codes */ +# define ATIF_QBTC_ERROR_CODE_SUCCESS 0 +# define ATIF_QBTC_ERROR_CODE_FAILURE 1 +# define ATIF_QBTC_ERROR_CODE_DEVICE_NOT_SUPPORTED 2 +struct atif_brightness_input { + uint16_t size; + /* ATIF doc indicates this field is a word, but the kernel drivers uses a byte. */ + uint8_t requested_display; +}; +struct atif_brightness_output { + uint16_t size; /* Size of this object, including size field. */ + uint16_t flags; /* Currently all reserved. */ + uint8_t error_code; + /* default brightness fields currently ignored by Linux driver. */ + uint8_t default_brightness_ac; /* Percentage brightness when connected to AC. */ + uint8_t default_brightness_dc; /* Percentage brightness when connected to DC. */ + /* The following 2 fields are the only ones honored by Linux driver currently. */ + uint8_t min_input_signal_level; /* 0-255 corresponding to 0% */ + uint8_t max_input_signal_level; /* 0-255 corresponding to 100% */ + /* Array of data points consisting of: + * { uint8_t luminance_level; (percent) + * uint8_t input_signal_level; (0-255 in value) } + * Linux ignores these fields so no support currently. */ + uint8_t count_data_points; /* Count of data points. */ +}; + +static void generate_atif(const struct device *dev) +{ + struct atif_verify_interface_output verify_output = { + .size = sizeof(verify_output), + .version = 1, + .supported_functions = + BIT(ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS - 1), + }; + struct atif_brightness_output brightness_error = { + .size = sizeof(brightness_error), + .error_code = ATIF_QBTC_ERROR_CODE_DEVICE_NOT_SUPPORTED, + }; + struct atif_brightness_output brightness_out = { + .size = sizeof(brightness_out), + .error_code = ATIF_QBTC_ERROR_CODE_SUCCESS, + .min_input_signal_level = 0, + .max_input_signal_level = 255, + }; + + /* Scope (_SB.PCI0.PBRA.IGFX) */ + acpigen_write_scope(acpi_device_path(dev)); + /* Method (ATIF, 2, NotSerialized) */ + acpigen_write_method("ATIF", 2); + /* ToInteger (Arg0, Local0) */ + acpigen_write_to_integer(ARG0_OP, LOCAL0_OP); + + /* If ((Local0 == Zero)) */ + acpigen_write_if_lequal_op_int(LOCAL0_OP, ATIF_FUNCTION_VERIFY_INTERFACE); + /* Return (Buffer (0x0C) { ... } */ + acpigen_write_return_byte_buffer((uint8_t *)(void *)&verify_output, + sizeof(verify_output)); + acpigen_pop_len(); /* if (LEqual(Local0, 0) */ + + /* ElseIf ((Local0 == 0x10)) */ + acpigen_write_else(); + acpigen_write_if_lequal_op_int(LOCAL0_OP, + ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS); + /* CreateByteField (Arg1, 0x02, DISP) */ + acpigen_write_create_byte_field(ARG1_OP, + offsetof(struct atif_brightness_input, requested_display), "DISP"); + /* ToInteger (DISP, Local1) */ + acpigen_write_to_integer_from_namestring("DISP", LOCAL1_OP); + /* If ((Local1 == Zero)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, ATIF_QBTC_REQUEST_LCD1); + /* Return (Buffer (0x0A) { ... } */ + acpigen_write_return_byte_buffer((uint8_t *)(void *)&brightness_out, + sizeof(brightness_out)); + acpigen_pop_len(); /* if (LEqual(Local2, ATIF_QBTC_REQUEST_LCD1) */ + /* Else */ + acpigen_write_else(); + /* Return (Buffer (0x0A) */ + acpigen_write_return_byte_buffer((uint8_t *)(void *)&brightness_error, + sizeof(brightness_error)); + acpigen_pop_len(); /* else */ + + acpigen_pop_len(); /* if (LEqual(Local0, 0x10) */ + acpigen_pop_len(); /* else */ + + acpigen_pop_len(); /* Method */ + acpigen_pop_len(); /* Scope */ +} + static void graphics_fill_ssdt(const struct device *dev) { acpi_device_write_pci_dev(dev); pci_rom_ssdt(dev); + if (CONFIG(SOC_AMD_COMMON_BLOCK_GRAPHICS_ATIF)) + generate_atif(dev); }
static const char *graphics_acpi_name(const struct device *dev)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44642 )
Change subject: soc/amd/common: add rudimentary ATIF support ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/44642/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44642/1//COMMIT_MSG@9 PS1, Line 9: kenerl drive kernel driver
https://review.coreboot.org/c/coreboot/+/44642/1/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/44642/1/src/soc/amd/common/block/gr... PS1, Line 101: Do we need a default else case to handle a call with unexpected Arg0? Or just assume that the kernel will do the right thing?
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44642
to look at the new patch set (#2).
Change subject: soc/amd/common: add rudimentary ATIF support ......................................................................
soc/amd/common: add rudimentary ATIF support
The Linux kenerl driver for AMD gpu currently has a floor value of 12 for brightness settings (AMDGPU_DM_DEFAULT_MIN_BACKLIGHT). AMD indicates they did this because they were concerned with certain panels flickering at lower backlight values. However, for unaffected panels it's desirable to be able to have the panel "turn off" at the lowest backlight setting. The only way to do that is to provide ATIF bindings that indicate backlight range.
Option SOC_AMD_COMMON_BLOCK_GRAPHICS_ATIF is added to provide a full range for the backlight setting. If needed, this path can be built upon for fuller support, but for the time being this is the only thing necessary to make the backlight be full range.
BUG=b:163583825
Change-Id: If76801a8daf6a5e56ba7d118956f3ebce74e567a Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/soc/amd/common/block/graphics/Kconfig M src/soc/amd/common/block/graphics/graphics.c 2 files changed, 109 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/44642/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44642 )
Change subject: soc/amd/common: add rudimentary ATIF support ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44642/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44642/1//COMMIT_MSG@9 PS1, Line 9: kenerl drive
kernel driver
Done
https://review.coreboot.org/c/coreboot/+/44642/1/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/44642/1/src/soc/amd/common/block/gr... PS1, Line 101:
Do we need a default else case to handle a call with unexpected Arg0? Or just assume that the kernel […]
There's no notion of a consistent return value. I don't know what we could do there. The function bitmap is used to determine what other Arg0 sub functions to call and we're publishing only support for the brightness.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44642 )
Change subject: soc/amd/common: add rudimentary ATIF support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44642/1/src/soc/amd/common/block/gr... File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/44642/1/src/soc/amd/common/block/gr... PS1, Line 101:
There's no notion of a consistent return value. I don't know what we could do there. […]
Makes sense. Yeah, I did not find any consistent return value that could indicate error. I think what we have here is good.
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44642 )
Change subject: soc/amd/common: add rudimentary ATIF support ......................................................................
soc/amd/common: add rudimentary ATIF support
The Linux kenerl driver for AMD gpu currently has a floor value of 12 for brightness settings (AMDGPU_DM_DEFAULT_MIN_BACKLIGHT). AMD indicates they did this because they were concerned with certain panels flickering at lower backlight values. However, for unaffected panels it's desirable to be able to have the panel "turn off" at the lowest backlight setting. The only way to do that is to provide ATIF bindings that indicate backlight range.
Option SOC_AMD_COMMON_BLOCK_GRAPHICS_ATIF is added to provide a full range for the backlight setting. If needed, this path can be built upon for fuller support, but for the time being this is the only thing necessary to make the backlight be full range.
BUG=b:163583825
Change-Id: If76801a8daf6a5e56ba7d118956f3ebce74e567a Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/44642 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/common/block/graphics/Kconfig M src/soc/amd/common/block/graphics/graphics.c 2 files changed, 109 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/common/block/graphics/Kconfig b/src/soc/amd/common/block/graphics/Kconfig index 8aa2a20..4cda353 100644 --- a/src/soc/amd/common/block/graphics/Kconfig +++ b/src/soc/amd/common/block/graphics/Kconfig @@ -3,3 +3,11 @@ default n help Select this option to use AMD common graphics driver support. + +config SOC_AMD_COMMON_BLOCK_GRAPHICS_ATIF + bool + depends on SOC_AMD_COMMON_BLOCK_GRAPHICS + help + Select this option to provide ATIF method with display brightness querying. + Currently, the exported values only open up 0-255 as the brightness range for + the display. diff --git a/src/soc/amd/common/block/graphics/graphics.c b/src/soc/amd/common/block/graphics/graphics.c index 466a91c..24cabfe 100644 --- a/src/soc/amd/common/block/graphics/graphics.c +++ b/src/soc/amd/common/block/graphics/graphics.c @@ -1,13 +1,114 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> #include <device/pci.h> #include <device/pci_ids.h>
+#define ATIF_FUNCTION_VERIFY_INTERFACE 0x0 +struct atif_verify_interface_output { + uint16_t size; /* Size of this object, including size field */ + uint16_t version; + uint32_t supported_notifications; + uint32_t supported_functions; /* Bit n set if function n+1 supported. */ +}; + +#define ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS 0x10 +# define ATIF_QBTC_REQUEST_LCD1 0 +/* error codes */ +# define ATIF_QBTC_ERROR_CODE_SUCCESS 0 +# define ATIF_QBTC_ERROR_CODE_FAILURE 1 +# define ATIF_QBTC_ERROR_CODE_DEVICE_NOT_SUPPORTED 2 +struct atif_brightness_input { + uint16_t size; + /* ATIF doc indicates this field is a word, but the kernel drivers uses a byte. */ + uint8_t requested_display; +}; +struct atif_brightness_output { + uint16_t size; /* Size of this object, including size field. */ + uint16_t flags; /* Currently all reserved. */ + uint8_t error_code; + /* default brightness fields currently ignored by Linux driver. */ + uint8_t default_brightness_ac; /* Percentage brightness when connected to AC. */ + uint8_t default_brightness_dc; /* Percentage brightness when connected to DC. */ + /* The following 2 fields are the only ones honored by Linux driver currently. */ + uint8_t min_input_signal_level; /* 0-255 corresponding to 0% */ + uint8_t max_input_signal_level; /* 0-255 corresponding to 100% */ + /* Array of data points consisting of: + * { uint8_t luminance_level; (percent) + * uint8_t input_signal_level; (0-255 in value) } + * Linux ignores these fields so no support currently. */ + uint8_t count_data_points; /* Count of data points. */ +}; + +static void generate_atif(const struct device *dev) +{ + struct atif_verify_interface_output verify_output = { + .size = sizeof(verify_output), + .version = 1, + .supported_functions = + BIT(ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS - 1), + }; + struct atif_brightness_output brightness_error = { + .size = sizeof(brightness_error), + .error_code = ATIF_QBTC_ERROR_CODE_DEVICE_NOT_SUPPORTED, + }; + struct atif_brightness_output brightness_out = { + .size = sizeof(brightness_out), + .error_code = ATIF_QBTC_ERROR_CODE_SUCCESS, + .min_input_signal_level = 0, + .max_input_signal_level = 255, + }; + + /* Scope (_SB.PCI0.PBRA.IGFX) */ + acpigen_write_scope(acpi_device_path(dev)); + /* Method (ATIF, 2, NotSerialized) */ + acpigen_write_method("ATIF", 2); + /* ToInteger (Arg0, Local0) */ + acpigen_write_to_integer(ARG0_OP, LOCAL0_OP); + + /* If ((Local0 == Zero)) */ + acpigen_write_if_lequal_op_int(LOCAL0_OP, ATIF_FUNCTION_VERIFY_INTERFACE); + /* Return (Buffer (0x0C) { ... } */ + acpigen_write_return_byte_buffer((uint8_t *)(void *)&verify_output, + sizeof(verify_output)); + acpigen_pop_len(); /* if (LEqual(Local0, 0) */ + + /* ElseIf ((Local0 == 0x10)) */ + acpigen_write_else(); + acpigen_write_if_lequal_op_int(LOCAL0_OP, + ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS); + /* CreateByteField (Arg1, 0x02, DISP) */ + acpigen_write_create_byte_field(ARG1_OP, + offsetof(struct atif_brightness_input, requested_display), "DISP"); + /* ToInteger (DISP, Local1) */ + acpigen_write_to_integer_from_namestring("DISP", LOCAL1_OP); + /* If ((Local1 == Zero)) */ + acpigen_write_if_lequal_op_int(LOCAL1_OP, ATIF_QBTC_REQUEST_LCD1); + /* Return (Buffer (0x0A) { ... } */ + acpigen_write_return_byte_buffer((uint8_t *)(void *)&brightness_out, + sizeof(brightness_out)); + acpigen_pop_len(); /* if (LEqual(Local2, ATIF_QBTC_REQUEST_LCD1) */ + /* Else */ + acpigen_write_else(); + /* Return (Buffer (0x0A) */ + acpigen_write_return_byte_buffer((uint8_t *)(void *)&brightness_error, + sizeof(brightness_error)); + acpigen_pop_len(); /* else */ + + acpigen_pop_len(); /* if (LEqual(Local0, 0x10) */ + acpigen_pop_len(); /* else */ + + acpigen_pop_len(); /* Method */ + acpigen_pop_len(); /* Scope */ +} + static void graphics_fill_ssdt(const struct device *dev) { acpi_device_write_pci_dev(dev); pci_rom_ssdt(dev); + if (CONFIG(SOC_AMD_COMMON_BLOCK_GRAPHICS_ATIF)) + generate_atif(dev); }
static const char *graphics_acpi_name(const struct device *dev)
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44642 )
Change subject: soc/amd/common: add rudimentary ATIF support ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 6/1/7 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/16090 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16089 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/16088 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16087 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/16086 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/16092 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16091
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Furquan Shaikh (Code Review)