From: Alex Deucher alexander.deucher@amd.com
Add a new header that defines the AMD ACPI interface used for laptops, PowerXpress, and chipset specific functionality and update the current code to use it.
Todo: - properly verify the ACPI interfaces - hook up and handle ACPI notifications - make PX code more robust - implement PCIe Gen and width switching using ACPI
Signed-off-by: Alex Deucher alexander.deucher@amd.com --- drivers/gpu/drm/radeon/radeon_acpi.c | 3 +- drivers/gpu/drm/radeon/radeon_acpi.h | 439 ++++++++++++++++++++++++++ drivers/gpu/drm/radeon/radeon_atpx_handler.c | 35 +-- 3 files changed, 455 insertions(+), 22 deletions(-) create mode 100644 drivers/gpu/drm/radeon/radeon_acpi.h
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c index 3516a60..5b32e49 100644 --- a/drivers/gpu/drm/radeon/radeon_acpi.c +++ b/drivers/gpu/drm/radeon/radeon_acpi.c @@ -9,6 +9,7 @@ #include "drm_sarea.h" #include "drm_crtc_helper.h" #include "radeon.h" +#include "radeon_acpi.h"
#include <linux/vga_switcheroo.h>
@@ -27,7 +28,7 @@ static int radeon_atif_call(acpi_handle handle) atif_arg.pointer = &atif_arg_elements[0];
atif_arg_elements[0].type = ACPI_TYPE_INTEGER; - atif_arg_elements[0].integer.value = 0; + atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE; atif_arg_elements[1].type = ACPI_TYPE_INTEGER; atif_arg_elements[1].integer.value = 0;
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.h b/drivers/gpu/drm/radeon/radeon_acpi.h new file mode 100644 index 0000000..a42288d --- /dev/null +++ b/drivers/gpu/drm/radeon/radeon_acpi.h @@ -0,0 +1,439 @@ +/* + * Copyright 2012 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + */ + +#ifndef RADEON_ACPI_H +#define RADEON_ACPI_H + +/* AMD hw uses four ACPI control methods: + * 1. ATIF + * ARG0: (ACPI_INTEGER) function code + * ARG1: (ACPI_BUFFER) parameter buffer, 256 bytes + * OUTPUT: (ACPI_BUFFER) output buffer, 256 bytes + * ATIF provides an entry point for the gfx driver to interact with the sbios. + * The AMD ACPI notification mechanism uses Notify (VGA, 0x81) or a custom + * notification. Which notification is used as indicated by the ATIF Control + * Method GET_SYSTEM_PARAMETERS. When the driver receives Notify (VGA, 0x81) or + * a custom notification it invokes ATIF Control Method GET_SYSTEM_BIOS_REQUESTS + * to identify pending System BIOS requests and associated parameters. For + * example, if one of the pending requests is DISPLAY_SWITCH_REQUEST, the driver + * will perform display device detection and invoke ATIF Control Method + * SELECT_ACTIVE_DISPLAYS. + * + * 2. ATPX + * ARG0: (ACPI_INTEGER) function code + * ARG1: (ACPI_BUFFER) parameter buffer, 256 bytes + * OUTPUT: (ACPI_BUFFER) output buffer, 256 bytes + * ATPX methods are used on PowerXpress systems to handle mux switching and + * discrete GPU power control. + * + * 3. ATRM + * ARG0: (ACPI_INTEGER) offset of vbios rom data + * ARG1: (ACPI_BUFFER) size of the buffer to fill (up to 4K). + * OUTPUT: (ACPI_BUFFER) output buffer + * ATRM provides an interfacess to access the discrete GPU vbios image on + * PowerXpress systems with multiple GPUs. + * + * 4. ATCS + * ARG0: (ACPI_INTEGER) function code + * ARG1: (ACPI_BUFFER) parameter buffer, 256 bytes + * OUTPUT: (ACPI_BUFFER) output buffer, 256 bytes + * ATCS provides an interface to AMD chipset specific functionality. + * + */ +/* ATIF */ +#define ATIF_FUNCTION_VERIFY_INTERFACE 0x0 +/* ARG0: ATIF_FUNCTION_VERIFY_INTERFACE + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * WORD - version + * DWORD - supported notifications mask + * DWORD - supported functions bit vector + */ +/* Notifications mask */ +# define ATIF_DISPLAY_SWITCH_REQUEST_SUPPORTED (1 << 0) +# define ATIF_EXPANSION_MODE_CHANGE_REQUEST_SUPPORTED (1 << 1) +# define ATIF_THERMAL_STATE_CHANGE_REQUEST_SUPPORTED (1 << 2) +# define ATIF_FORCED_POWER_STATE_CHANGE_REQUEST_SUPPORTED (1 << 3) +# define ATIF_SYSTEM_POWER_SOURCE_CHANGE_REQUEST_SUPPORTED (1 << 4) +# define ATIF_DISPLAY_CONF_CHANGE_REQUEST_SUPPORTED (1 << 5) +# define ATIF_PX_GFX_SWITCH_REQUEST_SUPPORTED (1 << 6) +# define ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST_SUPPORTED (1 << 7) +# define ATIF_DGPU_DISPLAY_EVENT_SUPPORTED (1 << 8) +/* supported functions vector */ +# define ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED (1 << 0) +# define ATIF_GET_SYSTEM_BIOS_REQUESTS_SUPPORTED (1 << 1) +# define ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) +# define ATIF_GET_LID_STATE_SUPPORTED (1 << 3) +# define ATIF_GET_TV_STANDARD_FROM_CMOS_SUPPORTED (1 << 4) +# define ATIF_SET_TV_STANDARD_IN_CMOS_SUPPORTED (1 << 5) +# define ATIF_GET_PANEL_EXPANSION_MODE_FROM_CMOS_SUPPORTED (1 << 6) +# define ATIF_SET_PANEL_EXPANSION_MODE_IN_CMOS_SUPPORTED (1 << 7) +# define ATIF_TEMPERATURE_CHANGE_NOTIFICATION_SUPPORTED (1 << 12) +# define ATIF_GET_GRAPHICS_DEVICE_TYPES_SUPPORTED (1 << 14) +#define ATIF_FUNCTION_GET_SYSTEM_PARAMETERS 0x1 +/* ARG0: ATIF_FUNCTION_GET_SYSTEM_PARAMETERS + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * DWORD - valid flags mask + * DWORD - flags + * + * OR + * + * WORD - structure size in bytes (includes size field) + * DWORD - valid flags mask + * DWORD - flags + * BYTE - notify command code + * + * flags + * bits 1:0: + * 0 - Notify(VGA, 0x81) is not used for notification + * 1 - Notify(VGA, 0x81) is used for notification + * 2 - Notify(VGA, n) is used for notification where + * n (0xd0-0xd9) is specified in notify command code. + * bit 2: + * 1 - lid changes not reported though int10 + */ +#define ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS 0x2 +/* ARG0: ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * DWORD - pending sbios requests + * BYTE - panel expansion mode + * BYTE - thermal state: target gfx controller + * BYTE - thermal state: state id (0: exit state, non-0: state) + * BYTE - forced power state: target gfx controller + * BYTE - forced power state: state id + * BYTE - system power source + * BYTE - panel backlight level (0-255) + */ +/* pending sbios requests */ +# define ATIF_DISPLAY_SWITCH_REQUEST (1 << 0) +# define ATIF_EXPANSION_MODE_CHANGE_REQUEST (1 << 1) +# define ATIF_THERMAL_STATE_CHANGE_REQUEST (1 << 2) +# define ATIF_FORCED_POWER_STATE_CHANGE_REQUEST (1 << 3) +# define ATIF_SYSTEM_POWER_SOURCE_CHANGE_REQUEST (1 << 4) +# define ATIF_DISPLAY_CONF_CHANGE_REQUEST (1 << 5) +# define ATIF_PX_GFX_SWITCH_REQUEST (1 << 6) +# define ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST (1 << 7) +# define ATIF_DGPU_DISPLAY_EVENT (1 << 8) +/* panel expansion mode */ +# define ATIF_PANEL_EXPANSION_DISABLE 0 +# define ATIF_PANEL_EXPANSION_FULL 1 +# define ATIF_PANEL_EXPANSION_ASPECT 2 +/* target gfx controller */ +# define ATIF_TARGET_GFX_SINGLE 0 +# define ATIF_TARGET_GFX_PX_IGPU 1 +# define ATIF_TARGET_GFX_PX_DGPU 2 +/* system power source */ +# define ATIF_POWER_SOURCE_AC 1 +# define ATIF_POWER_SOURCE_DC 2 +# define ATIF_POWER_SOURCE_RESTRICTED_AC_1 3 +# define ATIF_POWER_SOURCE_RESTRICTED_AC_2 4 +#define ATIF_FUNCTION_SELECT_ACTIVE_DISPLAYS 0x3 +/* ARG0: ATIF_FUNCTION_SELECT_ACTIVE_DISPLAYS + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - selected displays + * WORD - connected displays + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * WORD - selected displays + */ +# define ATIF_LCD1 (1 << 0) +# define ATIF_CRT1 (1 << 1) +# define ATIF_TV (1 << 2) +# define ATIF_DFP1 (1 << 3) +# define ATIF_CRT2 (1 << 4) +# define ATIF_LCD2 (1 << 5) +# define ATIF_DFP2 (1 << 7) +# define ATIF_CV (1 << 8) +# define ATIF_DFP3 (1 << 9) +# define ATIF_DFP4 (1 << 10) +# define ATIF_DFP5 (1 << 11) +# define ATIF_DFP6 (1 << 12) +#define ATIF_FUNCTION_GET_LID_STATE 0x4 +/* ARG0: ATIF_FUNCTION_GET_LID_STATE + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * BYTE - lid state (0: open, 1: closed) + * + * GET_LID_STATE only works at boot and resume, for general lid + * status, use the kernel provided status + */ +#define ATIF_FUNCTION_GET_TV_STANDARD_FROM_CMOS 0x5 +/* ARG0: ATIF_FUNCTION_GET_TV_STANDARD_FROM_CMOS + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * BYTE - 0 + * BYTE - TV standard + */ +# define ATIF_TV_STD_NTSC 0 +# define ATIF_TV_STD_PAL 1 +# define ATIF_TV_STD_PALM 2 +# define ATIF_TV_STD_PAL60 3 +# define ATIF_TV_STD_NTSCJ 4 +# define ATIF_TV_STD_PALCN 5 +# define ATIF_TV_STD_PALN 6 +# define ATIF_TV_STD_SCART_RGB 9 +#define ATIF_FUNCTION_SET_TV_STANDARD_IN_CMOS 0x6 +/* ARG0: ATIF_FUNCTION_SET_TV_STANDARD_IN_CMOS + * ARG1: + * WORD - structure size in bytes (includes size field) + * BYTE - 0 + * BYTE - TV standard + * OUTPUT: none + */ +#define ATIF_FUNCTION_GET_PANEL_EXPANSION_MODE_FROM_CMOS 0x7 +/* ARG0: ATIF_FUNCTION_GET_PANEL_EXPANSION_MODE_FROM_CMOS + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * BYTE - panel expansion mode + */ +#define ATIF_FUNCTION_SET_PANEL_EXPANSION_MODE_IN_CMOS 0x8 +/* ARG0: ATIF_FUNCTION_SET_PANEL_EXPANSION_MODE_IN_CMOS + * ARG1: + * WORD - structure size in bytes (includes size field) + * BYTE - panel expansion mode + * OUTPUT: none + */ +#define ATIF_FUNCTION_TEMPERATURE_CHANGE_NOTIFICATION 0xD +/* ARG0: ATIF_FUNCTION_TEMPERATURE_CHANGE_NOTIFICATION + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - gfx controller id + * BYTE - current temperature (degress Celsius) + * OUTPUT: none + */ +#define ATIF_FUNCTION_GET_GRAPHICS_DEVICE_TYPES 0xF +/* ARG0: ATIF_FUNCTION_GET_GRAPHICS_DEVICE_TYPES + * ARG1: none + * OUTPUT: + * WORD - number of gfx devices + * WORD - device structure size in bytes (excludes device size field) + * DWORD - flags \ + * WORD - bus number } repeated structure + * WORD - device number / + */ +/* flags */ +# define ATIF_PX_REMOVABLE_GRAPHICS_DEVICE (1 << 0) +# define ATIF_XGP_PORT (1 << 1) +# define ATIF_VGA_ENABLED_GRAPHICS_DEVICE (1 << 2) +# define ATIF_XGP_PORT_IN_DOCK (1 << 3) + +/* ATPX */ +#define ATPX_FUNCTION_VERIFY_INTERFACE 0x0 +/* ARG0: ATPX_FUNCTION_VERIFY_INTERFACE + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * WORD - version + * DWORD - supported functions bit vector + */ +/* supported functions vector */ +# define ATPX_GET_PX_PARAMETERS_SUPPORTED (1 << 0) +# define ATPX_POWER_CONTROL_SUPPORTED (1 << 1) +# define ATPX_DISPLAY_MUX_CONTROL_SUPPORTED (1 << 2) +# define ATPX_I2C_MUX_CONTROL_SUPPORTED (1 << 3) +# define ATPX_GRAPHICS_DEVICE_SWITCH_START_NOTIFICATION_SUPPORTED (1 << 4) +# define ATPX_GRAPHICS_DEVICE_SWITCH_END_NOTIFICATION_SUPPORTED (1 << 5) +# define ATPX_GET_DISPLAY_CONNECTORS_MAPPING_SUPPORTED (1 << 7) +# define ATPX_GET_DISPLAY_DETECTION_PORTS_SUPPORTED (1 << 8) +#define ATPX_FUNCTION_GET_PX_PARAMETERS 0x1 +/* ARG0: ATPX_FUNCTION_GET_PX_PARAMETERS + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * DWORD - valid flags mask + * DWORD - flags + */ +/* flags */ +# define ATPX_LVDS_I2C_AVAILABLE_TO_BOTH_GPUS (1 << 0) +# define ATPX_CRT1_I2C_AVAILABLE_TO_BOTH_GPUS (1 << 1) +# define ATPX_DVI1_I2C_AVAILABLE_TO_BOTH_GPUS (1 << 2) +# define ATPX_CRT1_RGB_SIGNAL_MUXED (1 << 3) +# define ATPX_TV_SIGNAL_MUXED (1 << 4) +# define ATPX_DFP_SIGNAL_MUXED (1 << 5) +# define ATPX_SEPARATE_MUX_FOR_I2C (1 << 6) +# define ATPX_DYNAMIC_PX_SUPPORTED (1 << 7) +# define ATPX_ACF_NOT_SUPPORTED (1 << 8) +# define ATPX_FIXED_NOT_SUPPORTED (1 << 9) +# define ATPX_DYNAMIC_DGPU_POWER_OFF_SUPPORTED (1 << 10) +# define ATPX_DGPU_REQ_POWER_FOR_DISPLAYS (1 << 11) +#define ATPX_FUNCTION_POWER_CONTROL 0x2 +/* ARG0: ATPX_FUNCTION_POWER_CONTROL + * ARG1: + * WORD - structure size in bytes (includes size field) + * BYTE - dGPU power state (0: power off, 1: power on) + * OUTPUT: none + */ +#define ATPX_FUNCTION_DISPLAY_MUX_CONTROL 0x3 +/* ARG0: ATPX_FUNCTION_DISPLAY_MUX_CONTROL + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - display mux control (0: iGPU, 1: dGPU) + * OUTPUT: none + */ +# define ATPX_INTEGRATED_GPU 0 +# define ATPX_DISCRETE_GPU 1 +#define ATPX_FUNCTION_I2C_MUX_CONTROL 0x4 +/* ARG0: ATPX_FUNCTION_I2C_MUX_CONTROL + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - i2c/aux/hpd mux control (0: iGPU, 1: dGPU) + * OUTPUT: none + */ +#define ATPX_FUNCTION_GRAPHICS_DEVICE_SWITCH_START_NOTIFICATION 0x5 +/* ARG0: ATPX_FUNCTION_GRAPHICS_DEVICE_SWITCH_START_NOTIFICATION + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - target gpu (0: iGPU, 1: dGPU) + * OUTPUT: none + */ +#define ATPX_FUNCTION_GRAPHICS_DEVICE_SWITCH_END_NOTIFICATION 0x6 +/* ARG0: ATPX_FUNCTION_GRAPHICS_DEVICE_SWITCH_END_NOTIFICATION + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - target gpu (0: iGPU, 1: dGPU) + * OUTPUT: none + */ +#define ATPX_FUNCTION_GET_DISPLAY_CONNECTORS_MAPPING 0x8 +/* ARG0: ATPX_FUNCTION_GET_DISPLAY_CONNECTORS_MAPPING + * ARG1: none + * OUTPUT: + * WORD - number of display connectors + * WORD - connector structure size in bytes (excludes connector size field) + * BYTE - flags \ + * BYTE - ATIF display vector bit position } repeated + * BYTE - adapter id (0: iGPU, 1-n: dGPU ordered by pcie bus number) } structure + * WORD - connector ACPI id / + */ +/* flags */ +# define ATPX_DISPLAY_OUTPUT_SUPPORTED_BY_ADAPTER_ID_DEVICE (1 << 0) +# define ATPX_DISPLAY_HPD_SUPPORTED_BY_ADAPTER_ID_DEVICE (1 << 1) +# define ATPX_DISPLAY_I2C_SUPPORTED_BY_ADAPTER_ID_DEVICE (1 << 2) +#define ATPX_FUNCTION_GET_DISPLAY_DETECTION_PORTS 0x9 +/* ARG0: ATPX_FUNCTION_GET_DISPLAY_DETECTION_PORTS + * ARG1: none + * OUTPUT: + * WORD - number of HPD/DDC ports + * WORD - port structure size in bytes (excludes port size field) + * BYTE - ATIF display vector bit position \ + * BYTE - hpd id } reapeated structure + * BYTE - ddc id / + * + * available on A+A systems only + */ +/* hpd id */ +# define ATPX_HPD_NONE 0 +# define ATPX_HPD1 1 +# define ATPX_HPD2 2 +# define ATPX_HPD3 3 +# define ATPX_HPD4 4 +# define ATPX_HPD5 5 +# define ATPX_HPD6 6 +/* ddc id */ +# define ATPX_DDC_NONE 0 +# define ATPX_DDC1 1 +# define ATPX_DDC2 2 +# define ATPX_DDC3 3 +# define ATPX_DDC4 4 +# define ATPX_DDC5 5 +# define ATPX_DDC6 6 +# define ATPX_DDC7 7 +# define ATPX_DDC8 8 + +/* ATCS */ +#define ATCS_FUNCTION_VERIFY_INTERFACE 0x0 +/* ARG0: ATCS_FUNCTION_VERIFY_INTERFACE + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * WORD - version + * DWORD - supported functions bit vector + */ +/* supported functions vector */ +# define ATCS_GET_EXTERNAL_STATE_SUPPORTED (1 << 0) +# define ATCS_PCIE_PERFORMANCE_REQUEST_SUPPORTED (1 << 1) +# define ATCS_PCIE_DEVICE_READY_NOTIFICATION_SUPPORTED (1 << 2) +# define ATCS_SET_PCIE_BUS_WIDTH_SUPPORTED (1 << 3) +#define ATCS_FUNCTION_GET_EXTERNAL_STATE 0x1 +/* ARG0: ATCS_FUNCTION_GET_EXTERNAL_STATE + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * DWORD - valid flags mask + * DWORD - flags (0: undocked, 1: docked) + */ +/* flags */ +# define ATCS_DOCKED (1 << 0) +#define ATCS_FUNCTION_PCIE_PERFORMANCE_REQUEST 0x2 +/* ARG0: ATPX_FUNCTION_GRAPHICS_DEVICE_SWITCH_END_NOTIFICATION + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - client id (bit 2-0: func num, 7-3: dev num, 15-8: bus num) + * WORD - valid flags mask + * WORD - flags + * BYTE - request type + * BYTE - performance request + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * BYTE - return value + */ +/* flags */ +# define ATCS_ADVERTISE_CAPS (1 << 0) +# define ATCS_WAIT_FOR_COMPLETION (1 << 1) +/* request type */ +# define ATCS_PCIE_LINK_SPEED 1 +/* performance request */ +# define ATCS_REMOVE 0 +# define ATCS_FORCE_LOW_POWER 1 +# define ATCS_PERF_LEVEL_1 2 /* PCIE Gen 1 */ +# define ATCS_PERF_LEVEL_2 3 /* PCIE Gen 2 */ +# define ATCS_PERF_LEVEL_3 4 /* PCIE Gen 3 */ +/* return value */ +# define ATCS_REQUEST_REFUSED 1 +# define ATCS_REQUEST_COMPLETE 2 +# define ATCS_REQUEST_IN_PROGRESS 3 +#define ATCS_FUNCTION_PCIE_DEVICE_READY_NOTIFICATION 0x3 +/* ARG0: ATCS_FUNCTION_PCIE_DEVICE_READY_NOTIFICATION + * ARG1: none + * OUTPUT: none + */ +#define ATCS_FUNCTION_SET_PCIE_BUS_WIDTH 0x4 +/* ARG0: ATCS_FUNCTION_SET_PCIE_BUS_WIDTH + * ARG1: + * WORD - structure size in bytes (includes size field) + * WORD - client id (bit 2-0: func num, 7-3: dev num, 15-8: bus num) + * BYTE - number of active lanes + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * BYTE - number of active lanes + */ + +#endif diff --git a/drivers/gpu/drm/radeon/radeon_atpx_handler.c b/drivers/gpu/drm/radeon/radeon_atpx_handler.c index 98724fc..ab1c4a9 100644 --- a/drivers/gpu/drm/radeon/radeon_atpx_handler.c +++ b/drivers/gpu/drm/radeon/radeon_atpx_handler.c @@ -12,18 +12,7 @@ #include <acpi/acpi_bus.h> #include <linux/pci.h>
-#define ATPX_VERSION 0 -#define ATPX_GPU_PWR 2 -#define ATPX_MUX_SELECT 3 -#define ATPX_I2C_MUX_SELECT 4 -#define ATPX_SWITCH_START 5 -#define ATPX_SWITCH_END 6 - -#define ATPX_INTEGRATED 0 -#define ATPX_DISCRETE 1 - -#define ATPX_MUX_IGD 0 -#define ATPX_MUX_DISCRETE 1 +#include "radeon_acpi.h"
static struct radeon_atpx_priv { bool atpx_detected; @@ -92,10 +81,10 @@ static int radeon_atpx_get_version(acpi_handle handle) atpx_arg.pointer = &atpx_arg_elements[0];
atpx_arg_elements[0].type = ACPI_TYPE_INTEGER; - atpx_arg_elements[0].integer.value = ATPX_VERSION; + atpx_arg_elements[0].integer.value = ATPX_FUNCTION_VERIFY_INTERFACE;
atpx_arg_elements[1].type = ACPI_TYPE_INTEGER; - atpx_arg_elements[1].integer.value = ATPX_VERSION; + atpx_arg_elements[1].integer.value = 0;
status = acpi_evaluate_object(handle, NULL, &atpx_arg, &buffer); if (ACPI_FAILURE(status)) { @@ -145,27 +134,31 @@ static int radeon_atpx_execute(acpi_handle handle, int cmd_id, u16 value)
static int radeon_atpx_set_discrete_state(acpi_handle handle, int state) { - return radeon_atpx_execute(handle, ATPX_GPU_PWR, state); + return radeon_atpx_execute(handle, ATPX_FUNCTION_POWER_CONTROL, state); }
static int radeon_atpx_switch_mux(acpi_handle handle, int mux_id) { - return radeon_atpx_execute(handle, ATPX_MUX_SELECT, mux_id); + return radeon_atpx_execute(handle, ATPX_FUNCTION_DISPLAY_MUX_CONTROL, mux_id); }
static int radeon_atpx_switch_i2c_mux(acpi_handle handle, int mux_id) { - return radeon_atpx_execute(handle, ATPX_I2C_MUX_SELECT, mux_id); + return radeon_atpx_execute(handle, ATPX_FUNCTION_I2C_MUX_CONTROL, mux_id); }
static int radeon_atpx_switch_start(acpi_handle handle, int gpu_id) { - return radeon_atpx_execute(handle, ATPX_SWITCH_START, gpu_id); + return radeon_atpx_execute(handle, + ATPX_FUNCTION_GRAPHICS_DEVICE_SWITCH_START_NOTIFICATION, + gpu_id); }
static int radeon_atpx_switch_end(acpi_handle handle, int gpu_id) { - return radeon_atpx_execute(handle, ATPX_SWITCH_END, gpu_id); + return radeon_atpx_execute(handle, + ATPX_FUNCTION_GRAPHICS_DEVICE_SWITCH_END_NOTIFICATION, + gpu_id); }
static int radeon_atpx_switchto(enum vga_switcheroo_client_id id) @@ -173,9 +166,9 @@ static int radeon_atpx_switchto(enum vga_switcheroo_client_id id) int gpu_id;
if (id == VGA_SWITCHEROO_IGD) - gpu_id = ATPX_INTEGRATED; + gpu_id = ATPX_INTEGRATED_GPU; else - gpu_id = ATPX_DISCRETE; + gpu_id = ATPX_DISCRETE_GPU;
radeon_atpx_switch_start(radeon_atpx_priv.atpx_handle, gpu_id); radeon_atpx_switch_mux(radeon_atpx_priv.atpx_handle, gpu_id);
On Wed, Jul 25, 2012 at 01:38:09PM -0400, alexdeucher@gmail.com wrote:
From: Alex Deucher alexander.deucher@amd.com
Add a new header that defines the AMD ACPI interface used for laptops, PowerXpress, and chipset specific functionality and update the current code to use it.
Great! Now my DSDT makes sense ;)
Todo:
- properly verify the ACPI interfaces
- hook up and handle ACPI notifications
I see a problem here, I've hacked up a standalone test module, but:
+#define ATIF_FUNCTION_GET_SYSTEM_PARAMETERS 0x1
[...]
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
My system has this bit set, and the brightness control method does send the notification. My module register itself with register_acpi_notifier and gets 2 notifications, one with ACPI_VIDEO_NOTIFY_PROBE (0x81) and the other with ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS (or DEC, depending on what I press).
The standard acpi "video" module, however, in response to ACPI_VIDEO_NOTIFY_PROBE generates a key press sending KEY_SWITCHVIDEOMODE.
This greatly confuses KDE which messes up my dual screen configuration; I guess that the spurious KEY_SWITCHVIDEOMODE may be problematic also with other DEs...
In more detail what happens is the following: - I press the brightness hotkey, firmware generates a notification on the relevant device (_SB.PCI0.PEG0.VGA.LCD) - ACPI video module gets the ACPI_VIDEO_NOTIFY_{DEC,INC}_BRIGHTNESS notification and tries to adjust the brightness with acpi_video_device_lcd_set_level, which in turns calls _BCM - _BCM sets the relevant bits in the AMD-specific structure and does a Notify (VGA, 0x81) - again ACPI video module gets the nodification (in this case ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE - KDE seems this and muck with the screen configuration :( - meanwhile the brightness notification is propagated, the hypothetical radeon driver does its magic to adjust the screen.
My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the acpi notifier chain, and allow the handlers to veto the key press (like it's done for ACPI_VIDEO_NOTIFY_SWITCH).
Zhang Rui what do you think about this?
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
My card is a:
01:00.0 VGA compatible controller [0300]: Advanced Micro Devices [AMD] nee ATI Thames XT/GL [Radeon HD 600M Series] [1002:6840]
on a Toshiba L855.
Luca
On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Wed, Jul 25, 2012 at 01:38:09PM -0400, alexdeucher@gmail.com wrote:
From: Alex Deucher alexander.deucher@amd.com
Add a new header that defines the AMD ACPI interface used for laptops, PowerXpress, and chipset specific functionality and update the current code to use it.
Great! Now my DSDT makes sense ;)
Todo:
- properly verify the ACPI interfaces
- hook up and handle ACPI notifications
I see a problem here, I've hacked up a standalone test module, but:
+#define ATIF_FUNCTION_GET_SYSTEM_PARAMETERS 0x1
[...]
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
My system has this bit set, and the brightness control method does send the notification. My module register itself with register_acpi_notifier and gets 2 notifications, one with ACPI_VIDEO_NOTIFY_PROBE (0x81) and the other with ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS (or DEC, depending on what I press).
The standard acpi "video" module, however, in response to ACPI_VIDEO_NOTIFY_PROBE generates a key press sending KEY_SWITCHVIDEOMODE.
This greatly confuses KDE which messes up my dual screen configuration; I guess that the spurious KEY_SWITCHVIDEOMODE may be problematic also with other DEs...
In more detail what happens is the following:
- I press the brightness hotkey, firmware generates a notification on the relevant device (_SB.PCI0.PEG0.VGA.LCD)
- ACPI video module gets the ACPI_VIDEO_NOTIFY_{DEC,INC}_BRIGHTNESS notification and tries to adjust the brightness with acpi_video_device_lcd_set_level, which in turns calls _BCM
- _BCM sets the relevant bits in the AMD-specific structure and does a Notify (VGA, 0x81)
- again ACPI video module gets the nodification (in this case ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the hypothetical radeon driver does its magic to adjust the screen.
My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the acpi notifier chain, and allow the handlers to veto the key press (like it's done for ACPI_VIDEO_NOTIFY_SWITCH).
Zhang Rui what do you think about this?
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
You need to check if the GPU controls the backlight or the system does. I think the attached patches should point you in the right direction.
Alex
My card is a:
01:00.0 VGA compatible controller [0300]: Advanced Micro Devices [AMD] nee ATI Thames XT/GL [Radeon HD 600M Series] [1002:6840]
on a Toshiba L855.
Luca
On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti kronos.it@gmail.com wrote:
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
You need to check if the GPU controls the backlight or the system does. I think the attached patches should point you in the right direction.
Yep :)
0050: ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability : 0050: (union) ATOM_FIRMWARE_CAPABILITY sbfAccess : USHORT GPUControlsBL:1 = 0x0001 (1)
The panel is using the INTERNAL_UNIPHY encoder, and I see the UNIPHYTransmitterControl command table.
Interaction with video.ko is still a bit messy...
Do you already have code for handling the notifications? I'll work on it in the weekend otherwise ;)
Luca
On Thu, Jul 26, 2012 at 3:33 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti kronos.it@gmail.com wrote:
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
You need to check if the GPU controls the backlight or the system does. I think the attached patches should point you in the right direction.
Yep :)
0050: ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability : 0050: (union) ATOM_FIRMWARE_CAPABILITY sbfAccess : USHORT GPUControlsBL:1 = 0x0001 (1)
The panel is using the INTERNAL_UNIPHY encoder, and I see the UNIPHYTransmitterControl command table.
Interaction with video.ko is still a bit messy...
Do you already have code for handling the notifications? I'll work on it in the weekend otherwise ;)
I don't have patches for that. Please feel free to work on it :)
Thanks,
Alex
On Thu, Jul 26, 2012 at 3:42 PM, Alex Deucher alexdeucher@gmail.com wrote:
On Thu, Jul 26, 2012 at 3:33 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti kronos.it@gmail.com wrote:
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
You need to check if the GPU controls the backlight or the system does. I think the attached patches should point you in the right direction.
Yep :)
0050: ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability : 0050: (union) ATOM_FIRMWARE_CAPABILITY sbfAccess : USHORT GPUControlsBL:1 = 0x0001 (1)
The panel is using the INTERNAL_UNIPHY encoder, and I see the UNIPHYTransmitterControl command table.
Interaction with video.ko is still a bit messy...
Do you already have code for handling the notifications? I'll work on it in the weekend otherwise ;)
I don't have patches for that. Please feel free to work on it :)
One thing worth checking, the sbios may write the requested backlight level to the bios scratch reg, in which case the driver only has to execute the BL_BRIGHTNESS action rather than writing the requested level to the register first.
Alex
Thanks,
Alex
On Thu, Jul 26, 2012 at 03:42:26PM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 3:33 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti kronos.it@gmail.com wrote:
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
You need to check if the GPU controls the backlight or the system does. I think the attached patches should point you in the right direction.
Yep :)
0050: ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability : 0050: (union) ATOM_FIRMWARE_CAPABILITY sbfAccess : USHORT GPUControlsBL:1 = 0x0001 (1)
The panel is using the INTERNAL_UNIPHY encoder, and I see the UNIPHYTransmitterControl command table.
Interaction with video.ko is still a bit messy...
Do you already have code for handling the notifications? I'll work on it in the weekend otherwise ;)
I don't have patches for that. Please feel free to work on it :)
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
thanks, Luca
On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Jul 26, 2012 at 03:42:26PM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 3:33 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti kronos.it@gmail.com wrote:
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
You need to check if the GPU controls the backlight or the system does. I think the attached patches should point you in the right direction.
Yep :)
0050: ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability : 0050: (union) ATOM_FIRMWARE_CAPABILITY sbfAccess : USHORT GPUControlsBL:1 = 0x0001 (1)
The panel is using the INTERNAL_UNIPHY encoder, and I see the UNIPHYTransmitterControl command table.
Interaction with video.ko is still a bit messy...
Do you already have code for handling the notifications? I'll work on it in the weekend otherwise ;)
I don't have patches for that. Please feel free to work on it :)
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
The spec says that the bits in the supported functions vector mean that if bit n is set, function n+1 exists, but it's possible that the spec is wrong and it's actually a 1 to 1 mapping; if bit n is set, function n is supported. In which case the the supported functions vector bits should be: +/* supported functions vector */ +# define ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED (1 << 1) +# define ATIF_GET_SYSTEM_BIOS_REQUESTS_SUPPORTED (1 << 2) +# define ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 3) +# define ATIF_GET_LID_STATE_SUPPORTED (1 << 4) +# define ATIF_GET_TV_STANDARD_FROM_CMOS_SUPPORTED (1 << 5) +# define ATIF_SET_TV_STANDARD_IN_CMOS_SUPPORTED (1 << 6) +# define ATIF_GET_PANEL_EXPANSION_MODE_FROM_CMOS_SUPPORTED (1 << 7) +# define ATIF_SET_PANEL_EXPANSION_MODE_IN_CMOS_SUPPORTED (1 << 8) +# define ATIF_TEMPERATURE_CHANGE_NOTIFICATION_SUPPORTED (1 << 13) +# define ATIF_GET_GRAPHICS_DEVICE_TYPES_SUPPORTED (1 << 15)
See if that lines up better. I'm still new to these ACPI interfaces so I'm not an expert yet.
Alex
On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti kronos.it@gmail.com wrote:
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
The spec says that the bits in the supported functions vector mean that if bit n is set, function n+1 exists,
Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
Maybe if the bit n is set then functions 0..n are available? That would (almost) match what I see...
but it's possible that the spec is wrong and it's actually a 1 to 1 mapping; if bit n is set, function n is supported. In which case the the supported functions vector bits should be: +/* supported functions vector */ +# define ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED (1 << 1) +# define ATIF_GET_SYSTEM_BIOS_REQUESTS_SUPPORTED (1 << 2) +# define ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 3) +# define ATIF_GET_LID_STATE_SUPPORTED (1 << 4) +# define ATIF_GET_TV_STANDARD_FROM_CMOS_SUPPORTED (1 << 5) +# define ATIF_SET_TV_STANDARD_IN_CMOS_SUPPORTED (1 << 6) +# define ATIF_GET_PANEL_EXPANSION_MODE_FROM_CMOS_SUPPORTED (1 << 7) +# define ATIF_SET_PANEL_EXPANSION_MODE_IN_CMOS_SUPPORTED (1 << 8) +# define ATIF_TEMPERATURE_CHANGE_NOTIFICATION_SUPPORTED (1 << 13) +# define ATIF_GET_GRAPHICS_DEVICE_TYPES_SUPPORTED (1 << 15)
See if that lines up better.
Not really... the value returned by VERIFY_INTERFACE is 0x2, but in the DSDT I see:
ATIF_FUNCTION_GET_SYSTEM_PARAMETERS ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS ATIF_FUNCTION_GET_TV_STANDARD_FROM_CMOS ATIF_FUNCTION_SET_TV_STANDARD_IN_CMOS
The implementation of the first one makes sense, the second is used for brightness control. The other two _might_ be a leftover (the machine does not have an analog TV out).
I'm still new to these ACPI interfaces so I'm not an expert yet.
I've been exposed to a lot of ACPI code (I wrote the asus_atk0110 driver), in my experience the DSDT is full of crap: code copied&pasted from other machines, leftover no longer used, and other stuff that's plainly wrong.
Luca
On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti kronos.it@gmail.com wrote:
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
The spec says that the bits in the supported functions vector mean that if bit n is set, function n+1 exists,
Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
Maybe if the bit n is set then functions 0..n are available? That would (almost) match what I see...
From the spec:
Supported DWORD Bit vector providing supported functions information. Each bit marks Functions Bit support for one specific function of the ATIF method. Bit n, if set, Vector indicates that Function n+1 is supported.
I don't know how wonky bioses in the wild are however. I can see what our internal teams do in these sort of cases.
but it's possible that the spec is wrong and it's actually a 1 to 1 mapping; if bit n is set, function n is supported. In which case the the supported functions vector bits should be: +/* supported functions vector */ +# define ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED (1 << 1) +# define ATIF_GET_SYSTEM_BIOS_REQUESTS_SUPPORTED (1 << 2) +# define ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 3) +# define ATIF_GET_LID_STATE_SUPPORTED (1 << 4) +# define ATIF_GET_TV_STANDARD_FROM_CMOS_SUPPORTED (1 << 5) +# define ATIF_SET_TV_STANDARD_IN_CMOS_SUPPORTED (1 << 6) +# define ATIF_GET_PANEL_EXPANSION_MODE_FROM_CMOS_SUPPORTED (1 << 7) +# define ATIF_SET_PANEL_EXPANSION_MODE_IN_CMOS_SUPPORTED (1 << 8) +# define ATIF_TEMPERATURE_CHANGE_NOTIFICATION_SUPPORTED (1 << 13) +# define ATIF_GET_GRAPHICS_DEVICE_TYPES_SUPPORTED (1 << 15)
See if that lines up better.
Not really... the value returned by VERIFY_INTERFACE is 0x2, but in the DSDT I see:
ATIF_FUNCTION_GET_SYSTEM_PARAMETERS ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS ATIF_FUNCTION_GET_TV_STANDARD_FROM_CMOS ATIF_FUNCTION_SET_TV_STANDARD_IN_CMOS
The implementation of the first one makes sense, the second is used for brightness control. The other two _might_ be a leftover (the machine does not have an analog TV out).
Yeah, probably unless there is a TV-out on a docking station or something like that. The TV-out port should show up in the connector table in the vbios even if it has a port on the docking station however.
Alex
On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti kronos.it@gmail.com wrote:
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
The spec says that the bits in the supported functions vector mean that if bit n is set, function n+1 exists,
Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
Maybe if the bit n is set then functions 0..n are available? That would (almost) match what I see...
From the spec:
Supported DWORD Bit vector providing supported functions information. Each bit marks Functions Bit support for one specific function of the ATIF method. Bit n, if set, Vector indicates that Function n+1 is supported.
Sorry, I still don't understand it... what's "Function n+1" in this context? Does this mean that if bit n is set then the function defined as 1 << (n+1) is supported?
I don't know how wonky bioses in the wild are however. I can see what our internal teams do in these sort of cases.
That would be helpful :) Note that on this machine (Toshiba L855-10W) brightness control under windows does not work with the stock catalyst driver, it works only with the (oldish) driver supplied by Toshiba.
Anyway, I'm attaching v2 of my patches, I've incorporated the suggestions and some bits of code from joeyli, and now brightness control is actually implemented.
Still missing is the issue of event 0x81 and the conflict with video.ko; the handler should probably return NOTIFY_BAD to veto the keypress.
Luca
On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti kronos.it@gmail.com wrote:
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
The spec says that the bits in the supported functions vector mean that if bit n is set, function n+1 exists,
Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
Maybe if the bit n is set then functions 0..n are available? That would (almost) match what I see...
From the spec:
Supported DWORD Bit vector providing supported functions information. Each bit marks Functions Bit support for one specific function of the ATIF method. Bit n, if set, Vector indicates that Function n+1 is supported.
Sorry, I still don't understand it... what's "Function n+1" in this context? Does this mean that if bit n is set then the function defined as 1 << (n+1) is supported?
It means if bit n is set in teh supported function vector, function n+1 is supported. E.g., if bit 1 is set, function 2 (ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS) is supported. If bit 3 is set function 4 (ATIF_FUNCTION_GET_LID_STATE) is supported, etc.
Alex
I don't know how wonky bioses in the wild are however. I can see what our internal teams do in these sort of cases.
That would be helpful :) Note that on this machine (Toshiba L855-10W) brightness control under windows does not work with the stock catalyst driver, it works only with the (oldish) driver supplied by Toshiba.
Anyway, I'm attaching v2 of my patches, I've incorporated the suggestions and some bits of code from joeyli, and now brightness control is actually implemented.
Still missing is the issue of event 0x81 and the conflict with video.ko; the handler should probably return NOTIFY_BAD to veto the keypress.
Luca
On Mon, Jul 30, 2012 at 10:30 PM, Alex Deucher alexdeucher@gmail.com wrote:
On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
Supported DWORD Bit vector providing supported functions information. Each bit marks Functions Bit support for one specific function of the ATIF method. Bit n, if set, Vector indicates that Function n+1 is supported.
Sorry, I still don't understand it... what's "Function n+1" in this context? Does this mean that if bit n is set then the function defined as 1 << (n+1) is supported?
It means if bit n is set in teh supported function vector, function n+1 is supported. E.g., if bit 1 is set, function 2 (ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS) is supported. If bit 3 is set function 4 (ATIF_FUNCTION_GET_LID_STATE) is supported, etc.
Great, just had an epiphany ;-) "n+1" refers to the value that's passed down to ATIF, I was still thinking in terms of bitmasks... Ok, so my code is correct, BIOS is botched... meh.
L
On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti kronos.it@gmail.com wrote:
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
The spec says that the bits in the supported functions vector mean that if bit n is set, function n+1 exists,
Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
Maybe if the bit n is set then functions 0..n are available? That would (almost) match what I see...
From the spec:
Supported DWORD Bit vector providing supported functions information. Each bit marks Functions Bit support for one specific function of the ATIF method. Bit n, if set, Vector indicates that Function n+1 is supported.
Sorry, I still don't understand it... what's "Function n+1" in this context? Does this mean that if bit n is set then the function defined as 1 << (n+1) is supported?
I don't know how wonky bioses in the wild are however. I can see what our internal teams do in these sort of cases.
That would be helpful :) Note that on this machine (Toshiba L855-10W) brightness control under windows does not work with the stock catalyst driver, it works only with the (oldish) driver supplied by Toshiba.
Anyway, I'm attaching v2 of my patches, I've incorporated the suggestions and some bits of code from joeyli, and now brightness control is actually implemented.
Regarding patches 3 and 4, it might be easier to just store a pointer to the relevant encoder when you verify ATIF rather than walking the encoder list every time.
Alex
Still missing is the issue of event 0x81 and the conflict with video.ko; the handler should probably return NOTIFY_BAD to veto the keypress.
Luca
On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher alexdeucher@gmail.com wrote:
Regarding patches 3 and 4, it might be easier to just store a pointer to the relevant encoder when you verify ATIF rather than walking the encoder list every time.
Makes sense, I was unsure about the lifetime of the encoders, but AFAICS they're destroyed only when the module in unloaded.
Luca
On Tue, Jul 31, 2012 at 5:16 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher alexdeucher@gmail.com wrote:
Regarding patches 3 and 4, it might be easier to just store a pointer to the relevant encoder when you verify ATIF rather than walking the encoder list every time.
Makes sense, I was unsure about the lifetime of the encoders, but AFAICS they're destroyed only when the module in unloaded.
They are present for the life of the driver.
Alex
On Tue, Jul 31, 2012 at 09:58:04AM -0400, Alex Deucher wrote:
On Tue, Jul 31, 2012 at 5:16 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher alexdeucher@gmail.com wrote:
Regarding patches 3 and 4, it might be easier to just store a pointer to the relevant encoder when you verify ATIF rather than walking the encoder list every time.
Done.
Makes sense, I was unsure about the lifetime of the encoders, but AFAICS they're destroyed only when the module in unloaded.
They are present for the life of the driver.
I had to move to call to radeon_acpi_init after radeon_modeset_init, otherwise the encoders are not present yet (the tear down code path is correct, acpi first, then modeset). Latest and greatest version is attached; I fixed notifications when using custom command codes (tested by Pali Rohár) and implemented your suggestion.
Luca
On Tue, Jul 31, 2012 at 4:05 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Tue, Jul 31, 2012 at 09:58:04AM -0400, Alex Deucher wrote:
On Tue, Jul 31, 2012 at 5:16 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher alexdeucher@gmail.com wrote:
Regarding patches 3 and 4, it might be easier to just store a pointer to the relevant encoder when you verify ATIF rather than walking the encoder list every time.
Done.
Makes sense, I was unsure about the lifetime of the encoders, but AFAICS they're destroyed only when the module in unloaded.
They are present for the life of the driver.
I had to move to call to radeon_acpi_init after radeon_modeset_init, otherwise the encoders are not present yet (the tear down code path is correct, acpi first, then modeset). Latest and greatest version is attached; I fixed notifications when using custom command codes (tested by Pali Rohár) and implemented your suggestion.
Patches look good. I picked them up and combined them with may patches plus a few other small fixes. They are available here: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches Let me know what you think.
Alex
On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
Patches look good. I picked them up and combined them with may patches plus a few other small fixes. They are available here: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches Let me know what you think.
Looks ok, I lost one fix along the road though, I'm attaching the patch.
Luca
On Wed, Aug 1, 2012 at 4:57 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
Patches look good. I picked them up and combined them with may patches plus a few other small fixes. They are available here: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches Let me know what you think.
Looks ok, I lost one fix along the road though, I'm attaching the patch.
Thanks, I rolled it into your original patch. New tree: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
Alex
Luca
I admit I'm not really an ACPI expert, but thinking about this more, I'm wondering if maybe we should just send the appropriate brightness change, switch display, etc. event to userspace rather than handling it directly in the radeon driver, then let userspace callback down via the bl interface, etc. With backlight for example, does handling it in the kernel driver as per your patch prevent userspace from seeing the brightness up/down event? Wouldn't that break things like OSD brightness displays and such?
Alex
On Wed, Aug 1, 2012 at 9:56 AM, Alex Deucher alexdeucher@gmail.com wrote:
On Wed, Aug 1, 2012 at 4:57 AM, Luca Tettamanti kronos.it@gmail.com wrote:
On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
Patches look good. I picked them up and combined them with may patches plus a few other small fixes. They are available here: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches Let me know what you think.
Looks ok, I lost one fix along the road though, I'm attaching the patch.
Thanks, I rolled it into your original patch. New tree: http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
Alex
Luca
On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher alexdeucher@gmail.com wrote:
I admit I'm not really an ACPI expert, but thinking about this more, I'm wondering if maybe we should just send the appropriate brightness change, switch display, etc. event to userspace rather than handling it directly in the radeon driver, then let userspace callback down via the bl interface, etc. With backlight for example, does handling it in the kernel driver as per your patch prevent userspace from seeing the brightness up/down event? Wouldn't that break things like OSD brightness displays and such?
No, the event is sent to userspace by the standard ACPI video driver, it works as before. Changing brightness usually goes like this: 1) user presses a hotkey 2) a notification is generated (0x86 or 0x87) 3) video.ko handles the notification and calls into ACPI to change the level (_BCM) and firmware does its magic 4) a key press (brightness up/down) is sent to userspace
With ATIF step 3 does not actually change the brightness, it just send out another event (VIDEO_PROBE, or one of the device specific ones) so we need to take care of that too. The rest of the process, including the delivery of the key presses, stays the same.
Luca
On Thu, Aug 2, 2012 at 12:31 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher alexdeucher@gmail.com wrote:
I admit I'm not really an ACPI expert, but thinking about this more, I'm wondering if maybe we should just send the appropriate brightness change, switch display, etc. event to userspace rather than handling it directly in the radeon driver, then let userspace callback down via the bl interface, etc. With backlight for example, does handling it in the kernel driver as per your patch prevent userspace from seeing the brightness up/down event? Wouldn't that break things like OSD brightness displays and such?
No, the event is sent to userspace by the standard ACPI video driver, it works as before. Changing brightness usually goes like this:
- user presses a hotkey
- a notification is generated (0x86 or 0x87)
- video.ko handles the notification and calls into ACPI to change the
level (_BCM) and firmware does its magic 4) a key press (brightness up/down) is sent to userspace
With ATIF step 3 does not actually change the brightness, it just send out another event (VIDEO_PROBE, or one of the device specific ones) so we need to take care of that too. The rest of the process, including the delivery of the key presses, stays the same.
Excellent! thanks for clarifying.
Alex
On Thu, Aug 2, 2012 at 12:33 PM, Alex Deucher alexdeucher@gmail.com wrote:
On Thu, Aug 2, 2012 at 12:31 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher alexdeucher@gmail.com wrote:
I admit I'm not really an ACPI expert, but thinking about this more, I'm wondering if maybe we should just send the appropriate brightness change, switch display, etc. event to userspace rather than handling it directly in the radeon driver, then let userspace callback down via the bl interface, etc. With backlight for example, does handling it in the kernel driver as per your patch prevent userspace from seeing the brightness up/down event? Wouldn't that break things like OSD brightness displays and such?
No, the event is sent to userspace by the standard ACPI video driver, it works as before. Changing brightness usually goes like this:
- user presses a hotkey
- a notification is generated (0x86 or 0x87)
- video.ko handles the notification and calls into ACPI to change the
level (_BCM) and firmware does its magic 4) a key press (brightness up/down) is sent to userspace
With ATIF step 3 does not actually change the brightness, it just send out another event (VIDEO_PROBE, or one of the device specific ones) so we need to take care of that too. The rest of the process, including the delivery of the key presses, stays the same.
Updated tree with fixes to a few existing patches and improved support for ATPX and initial support for ATCS: http://cgit.freedesktop.org/~agd5f/linux/?h=acpi_patches
Alex
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com --- Any comment from ACPI front?
drivers/acpi/video.c | 10 ++++++++-- drivers/gpu/drm/radeon/radeon_acpi.c | 7 ++++++- 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 1e0a9e1..a8592c4 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1457,7 +1457,12 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) acpi_video_device_enumerate(video); acpi_video_device_rebind(video); acpi_bus_generate_proc_event(device, event, 0); - keycode = KEY_SWITCHVIDEOMODE; + /* This event is also overloaded by AMD ACPI interface, don't + * send the key press if the event has been handled elsewhere + * (e.g. radeon DRM driver). + */ + if (!acpi_notifier_call_chain(device, event, 0)) + keycode = KEY_SWITCHVIDEOMODE; break;
case ACPI_VIDEO_NOTIFY_CYCLE: /* Cycle Display output hotkey pressed. */ @@ -1479,7 +1484,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) break; }
- if (event != ACPI_VIDEO_NOTIFY_SWITCH) + if (event != ACPI_VIDEO_NOTIFY_SWITCH && + event != ACPI_VIDEO_NOTIFY_PROBE) acpi_notifier_call_chain(device, event, 0);
if (keycode) { diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c index 96de08d..ee0d29e 100644 --- a/drivers/gpu/drm/radeon/radeon_acpi.c +++ b/drivers/gpu/drm/radeon/radeon_acpi.c @@ -273,7 +273,12 @@ int radeon_atif_handler(struct radeon_device *rdev, } /* TODO: check other events */
- return NOTIFY_OK; + /* We've handled the event, stop the notifier chain. The ACPI interface + * overloads ACPI_VIDEO_NOTIFY_PROBE, we don't want to send that to + * userspace if the event was generated only to signal a SBIOS + * request. + */ + return NOTIFY_BAD; }
/* Call all ACPI methods here */
On Wed, Aug 1, 2012 at 9:49 AM, Luca Tettamanti kronos.it@gmail.com wrote:
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com
Looks good to me, but I'm certainly not an ACPI expert.
Acked-by: Alex Deucher alexander.deucher@amd.com
於 三，2012-08-01 於 15:49 +0200，Luca Tettamanti 提到：
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com
Tested-by: Lee, Chun-Yi jlee@suse.com
This patch works to me on my HP notebook to avoid (VGA, 0x81) notify event issued when AC-power unpluged.
Thanks Joey Lee
Any comment from ACPI front?
drivers/acpi/video.c | 10 ++++++++-- drivers/gpu/drm/radeon/radeon_acpi.c | 7 ++++++- 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 1e0a9e1..a8592c4 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1457,7 +1457,12 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) acpi_video_device_enumerate(video); acpi_video_device_rebind(video); acpi_bus_generate_proc_event(device, event, 0);
keycode = KEY_SWITCHVIDEOMODE;
/* This event is also overloaded by AMD ACPI interface, don't* send the key press if the event has been handled elsewhere* (e.g. radeon DRM driver).*/if (!acpi_notifier_call_chain(device, event, 0))keycode = KEY_SWITCHVIDEOMODE;break;
case ACPI_VIDEO_NOTIFY_CYCLE: /* Cycle Display output hotkey pressed. */
@@ -1479,7 +1484,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) break; }
- if (event != ACPI_VIDEO_NOTIFY_SWITCH)
if (event != ACPI_VIDEO_NOTIFY_SWITCH &&
event != ACPI_VIDEO_NOTIFY_PROBE)acpi_notifier_call_chain(device, event, 0);
if (keycode) {
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c index 96de08d..ee0d29e 100644 --- a/drivers/gpu/drm/radeon/radeon_acpi.c +++ b/drivers/gpu/drm/radeon/radeon_acpi.c @@ -273,7 +273,12 @@ int radeon_atif_handler(struct radeon_device *rdev, } /* TODO: check other events */
- return NOTIFY_OK;
- /* We've handled the event, stop the notifier chain. The ACPI interface
* overloads ACPI_VIDEO_NOTIFY_PROBE, we don't want to send that to* userspace if the event was generated only to signal a SBIOS* request.*/- return NOTIFY_BAD;
}
/* Call all ACPI methods here */
On 三, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com
Any comment from ACPI front?
it looks good to me. But I'm wondering if we can use the following code for ACPI part, which looks cleaner. I know this may change the behavior of other events, but in theory, we should not send any input event if we know something wrong in kernel.
what do you think?
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 1e0a9e1..8ad1f53 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1448,8 +1448,7 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) case ACPI_VIDEO_NOTIFY_SWITCH: /* User requested a switch, * most likely via hotkey. */ acpi_bus_generate_proc_event(device, event, 0); - if (!acpi_notifier_call_chain(device, event, 0)) - keycode = KEY_SWITCHVIDEOMODE; + keycode = KEY_SWITCHVIDEOMODE; break;
case ACPI_VIDEO_NOTIFY_PROBE: /* User plugged in or removed a video @@ -1479,8 +1478,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) break; }
- if (event != ACPI_VIDEO_NOTIFY_SWITCH) - acpi_notifier_call_chain(device, event, 0); + if (acpi_notifier_call_chain(device, event, 0)) + keycode = 0;
if (keycode) { input_report_key(input, keycode, 1);
drivers/acpi/video.c | 10 ++++++++-- drivers/gpu/drm/radeon/radeon_acpi.c | 7 ++++++- 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 1e0a9e1..a8592c4 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1457,7 +1457,12 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) acpi_video_device_enumerate(video); acpi_video_device_rebind(video); acpi_bus_generate_proc_event(device, event, 0);
keycode = KEY_SWITCHVIDEOMODE;
/* This event is also overloaded by AMD ACPI interface, don't* send the key press if the event has been handled elsewhere* (e.g. radeon DRM driver).*/if (!acpi_notifier_call_chain(device, event, 0))keycode = KEY_SWITCHVIDEOMODE;break;
case ACPI_VIDEO_NOTIFY_CYCLE: /* Cycle Display output hotkey pressed. */
@@ -1479,7 +1484,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) break; }
- if (event != ACPI_VIDEO_NOTIFY_SWITCH)
if (event != ACPI_VIDEO_NOTIFY_SWITCH &&
event != ACPI_VIDEO_NOTIFY_PROBE)acpi_notifier_call_chain(device, event, 0);
if (keycode) {
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c index 96de08d..ee0d29e 100644 --- a/drivers/gpu/drm/radeon/radeon_acpi.c +++ b/drivers/gpu/drm/radeon/radeon_acpi.c @@ -273,7 +273,12 @@ int radeon_atif_handler(struct radeon_device *rdev, } /* TODO: check other events */
- return NOTIFY_OK;
- /* We've handled the event, stop the notifier chain. The ACPI interface
* overloads ACPI_VIDEO_NOTIFY_PROBE, we don't want to send that to* userspace if the event was generated only to signal a SBIOS* request.*/- return NOTIFY_BAD;
}
/* Call all ACPI methods here */
On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
On 三, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com
Any comment from ACPI front?
it looks good to me. But I'm wondering if we can use the following code for ACPI part, which looks cleaner. I know this may change the behavior of other events, but in theory, we should not send any input event if we know something wrong in kernel.
what do you think?
I like it, it's cleaner. I've split the patch in two pieces (one for video, the other for radeon) and adopted your suggestion.
BTW, I'm leaving for vacation in a few hours, I'll be offline till mid August.
Luca
On 四, 2012-08-02 at 15:46 +0200, Luca Tettamanti wrote:
On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
On 三, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com
Any comment from ACPI front?
it looks good to me. But I'm wondering if we can use the following code for ACPI part, which looks cleaner. I know this may change the behavior of other events, but in theory, we should not send any input event if we know something wrong in kernel.
what do you think?
I like it, it's cleaner. I've split the patch in two pieces (one for video, the other for radeon) and adopted your suggestion.
Great. Acked-by: Zhang Rui rui.zhang@intel.com
hmm, who should take these two patches? and which tree the second patch is based on?
thanks, rui
On Thu, Aug 2, 2012 at 9:40 PM, Zhang Rui rui.zhang@intel.com wrote:
On 四, 2012-08-02 at 15:46 +0200, Luca Tettamanti wrote:
On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
On 三, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com
Any comment from ACPI front?
it looks good to me. But I'm wondering if we can use the following code for ACPI part, which looks cleaner. I know this may change the behavior of other events, but in theory, we should not send any input event if we know something wrong in kernel.
what do you think?
I like it, it's cleaner. I've split the patch in two pieces (one for video, the other for radeon) and adopted your suggestion.
Great. Acked-by: Zhang Rui rui.zhang@intel.com
hmm, who should take these two patches?
I'm happy to take the patches.
and which tree the second patch is based on?
I've got a tree with all the radeon ACPI patches on the acpi_patches branches of my git tree: git://people.freedesktop.org/~agd5f/linux
Alex
thanks, rui
On 四, 2012-08-02 at 21:45 -0400, Alex Deucher wrote:
On Thu, Aug 2, 2012 at 9:40 PM, Zhang Rui rui.zhang@intel.com wrote:
On 四, 2012-08-02 at 15:46 +0200, Luca Tettamanti wrote:
On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
On 三, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
AMD ACPI interface may overload the standard event ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the userspace because the user did not press the mode switch key (the spurious keypress confuses the DE which usually changes the display configuration and messes up a dual-screen setup). This patch gives the radeon driver the chance to examine the event and block the keypress if the event is an "AMD event".
Signed-off-by: Luca Tettamanti kronos.it@gmail.com
Any comment from ACPI front?
it looks good to me. But I'm wondering if we can use the following code for ACPI part, which looks cleaner. I know this may change the behavior of other events, but in theory, we should not send any input event if we know something wrong in kernel.
what do you think?
I like it, it's cleaner. I've split the patch in two pieces (one for video, the other for radeon) and adopted your suggestion.
Great. Acked-by: Zhang Rui rui.zhang@intel.com
hmm, who should take these two patches?
I'm happy to take the patches.
and which tree the second patch is based on?
I've got a tree with all the radeon ACPI patches on the acpi_patches branches of my git tree: git://people.freedesktop.org/~agd5f/linux
great.
thanks, rui
Hi, I'm attaching a first draft of my work. The first 3 patches are infrastructure work, the fourth wires the notification handler and retrieves the requests from the system BIOS, but it does not actually change brightness yet.
The problem here is how to get the correct encoder: should I just scan encoder_list checking for ATOM_DEVICE_LCD_SUPPORT and see if it has a backlight device attached? Hopefully there is only one encoder with it, right?
I'm also toying with the idea of creating structures matching the output of the various ACPI methods, this would remove some ugly pointer arithmetics, but it _might_ make it easier to read past the buffer if one does not check the size before using the struct. What do you think?
Luca
On Sun, Jul 29, 2012 at 3:33 PM, Luca Tettamanti kronos.it@gmail.com wrote:
Hi, I'm attaching a first draft of my work. The first 3 patches are infrastructure work, the fourth wires the notification handler and retrieves the requests from the system BIOS, but it does not actually change brightness yet.
The problem here is how to get the correct encoder: should I just scan encoder_list checking for ATOM_DEVICE_LCD_SUPPORT and see if it has a backlight device attached?
Yeah, that should work.
Hopefully there is only one encoder with it, right?
There's only one backlight controller and there should only be one encoder with it enabled on it.
I'm also toying with the idea of creating structures matching the output of the various ACPI methods, this would remove some ugly pointer arithmetics, but it _might_ make it easier to read past the buffer if one does not check the size before using the struct. What do you think?
That's fine with me. We do something similar with atombios structs. Also, feel free to add stuff to radeon_acpi.h if you think it's useful.
Alex
Hi Luca,
於 六，2012-07-28 於 16:56 +0200，Luca Tettamanti 提到：
On Thu, Jul 26, 2012 at 03:42:26PM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 3:33 PM, Luca Tettamanti kronos.it@gmail.com wrote:
On Thu, Jul 26, 2012 at 11:35:25AM -0400, Alex Deucher wrote:
On Thu, Jul 26, 2012 at 8:58 AM, Luca Tettamanti kronos.it@gmail.com wrote:
The other missing bit is how to actually change the brightness... Alex, do you know what registers to poke?
You need to check if the GPU controls the backlight or the system does. I think the attached patches should point you in the right direction.
Yep :)
0050: ATOM_FIRMWARE_CAPABILITY_ACCESS usFirmwareCapability : 0050: (union) ATOM_FIRMWARE_CAPABILITY sbfAccess : USHORT GPUControlsBL:1 = 0x0001 (1)
The panel is using the INTERNAL_UNIPHY encoder, and I see the UNIPHYTransmitterControl command table.
Interaction with video.ko is still a bit messy...
Do you already have code for handling the notifications? I'll work on it in the weekend otherwise ;)
I don't have patches for that. Please feel free to work on it :)
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
thanks, Luca
Did you check your DSDT for there have some "Notify (VGA, 0x81)" statement in AFN0..AFN15? If YES, I think that means your machine in case the 0x81 is for ATI used by default.
On the other hand, I am also trying to write patch for avoid my AC-power problem. Like your idea, I think just add radeon-acpi to acpi notifier chain then acpi/video feed event to chain before issue KEY code like Matthew's code for ACPI_VIDEO_NOTIFY_SWITCH with intel_opregion on 0x80. The following code for reference:
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 1e0a9e1..fc138fd 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -91,6 +91,8 @@ static int acpi_video_bus_add(struct acpi_device *device); static int acpi_video_bus_remove(struct acpi_device *device, int type); static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
+static u16 video_notify_block_map; + static const struct acpi_device_id video_device_ids[] = { {ACPI_VIDEO_HID, 0}, {"", 0}, @@ -1457,7 +1459,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) acpi_video_device_enumerate(video); acpi_video_device_rebind(video); acpi_bus_generate_proc_event(device, event, 0); - keycode = KEY_SWITCHVIDEOMODE; + if (!acpi_notifier_call_chain(device, event, 0)) + keycode = KEY_SWITCHVIDEOMODE; break;
case ACPI_VIDEO_NOTIFY_CYCLE: /* Cycle Display output hotkey pressed. */ @@ -1479,7 +1482,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) break; }
- if (event != ACPI_VIDEO_NOTIFY_SWITCH) + if (event != ACPI_VIDEO_NOTIFY_SWITCH || + event != ACPI_VIDEO_NOTIFY_PROBE) acpi_notifier_call_chain(device, event, 0);
if (keycode) {
Thanks a lot! Joey Lee
On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
Hi Luca,
於 六，2012-07-28 於 16:56 +0200，Luca Tettamanti 提到：
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
Did you check your DSDT for there have some "Notify (VGA, 0x81)" statement in AFN0..AFN15? If YES, I think that means your machine in case the 0x81 is for ATI used by default.
Yes, my point is that the nofication is there, but since GET_SYSTEM_PARAMETERS is not announced I have not way to check it. IOW, what is implemented in the DSDT does not match what is announced by VERIFY_INTERFACE. For reference this is the DSDT: http://pastebin.com/KKS7ZsTt
Luca
於 日，2012-07-29 於 15:10 +0200，Luca Tettamanti 提到：
On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
Hi Luca,
於 六，2012-07-28 於 16:56 +0200，Luca Tettamanti 提到：
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
Did you check your DSDT for there have some "Notify (VGA, 0x81)" statement in AFN0..AFN15? If YES, I think that means your machine in case the 0x81 is for ATI used by default.
Yes, my point is that the nofication is there, but since GET_SYSTEM_PARAMETERS is not announced I have not way to check it. IOW, what is implemented in the DSDT does not match what is announced by VERIFY_INTERFACE. For reference this is the DSDT: http://pastebin.com/KKS7ZsTt
Luca
Yes, saw the problem in your DSDT:
Method (AF00, 0, NotSerialized) { CreateWordField (ATIB, Zero, SSZE) ... Store (0x80, NMSK) Store (0x02, SFUN) <=== 10b, bit 0 is 0 Return (ATIB) }
But, AF01 still supported in ATIF on this machine, maybe we should still try GET_SYSTEM_PARAMETERS even the function vector set to 0? No idea...
On the other hand, My patch to avoid 0x81 event conflict with acpi/video driver like below. This patch woks on my notebook. Your patches do much more things then mine, so I think my patch just for reference.
There have a problem is: If we want use acpi_notifier_call_chain to check does event consume by any notifier in acpi's blocking notifier chain, then we need return NOTIFY_BAD in radeon_acpi but not NOTIFY_OK.
So, I suggest radeon_acpi should register to acpi notifier chain by itself but not append on radeon_acpi_event in radeon_pm.
And, suggest also check the device class is ACPI_VIDEO_CLASS like following:
+static int radeon_acpi_video_event(struct notifier_block *nb, ... + if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0) + return NOTIFY_DONE; +
Thanks a lot! Joey Lee
From 9c0c42ab8f37dafd3b512ca395b64bf39819d26b Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi jlee@suse.com Date: Mon, 30 Jul 2012 16:17:05 +0800 Subject: [PATCH] drm/radeon avoid 0x81 acpi event conflict with acpi video driver
drm/radeon avoid 0x81 acpi event conflict with acpi video driver
Signed-off-by: Lee, Chun-Yi jlee@suse.com --- drivers/acpi/video.c | 6 +- drivers/gpu/drm/radeon/radeon_acpi.c | 150 ++++++++++++++++++++++++++++++---- 2 files changed, 139 insertions(+), 17 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 1e0a9e1..e32492d 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1457,7 +1457,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) acpi_video_device_enumerate(video); acpi_video_device_rebind(video); acpi_bus_generate_proc_event(device, event, 0); - keycode = KEY_SWITCHVIDEOMODE; + if (!acpi_notifier_call_chain(device, event, 0)) + keycode = KEY_SWITCHVIDEOMODE; break;
case ACPI_VIDEO_NOTIFY_CYCLE: /* Cycle Display output hotkey pressed. */ @@ -1479,7 +1480,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) break; }
- if (event != ACPI_VIDEO_NOTIFY_SWITCH) + if (event != ACPI_VIDEO_NOTIFY_SWITCH || + event != ACPI_VIDEO_NOTIFY_PROBE) acpi_notifier_call_chain(device, event, 0);
if (keycode) { diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c index 5b32e49..37ed5e1 100644 --- a/drivers/gpu/drm/radeon/radeon_acpi.c +++ b/drivers/gpu/drm/radeon/radeon_acpi.c @@ -3,6 +3,7 @@ #include <linux/slab.h> #include <acpi/acpi_drivers.h> #include <acpi/acpi_bus.h> +#include <acpi/video.h>
#include "drmP.h" #include "drm.h" @@ -13,36 +14,150 @@
#include <linux/vga_switcheroo.h>
+struct atif_verify_output { + u16 size; /* structure size in bytes (includes size field) */ + u16 version; /* version */ + u32 notif_mask; /* supported notifications mask */ + u32 func_bits; /* supported functions bit vector */ +} __attribute__((packed)); + +static struct atif_verify_output *verify_output; + +struct atif_get_sysparams_output { + u16 size; /* structure size in bytes (includes size field) */ + u32 valid_flags_mask; /* valid flags mask */ + u32 flags; /* flags */ + u8 notify_code; /* notify command code */ +} __attribute__((packed)); + +static u8 notify_code; + /* Call the ATIF method * * Note: currently we discard the output */ -static int radeon_atif_call(acpi_handle handle) +static int radeon_atif_call(acpi_handle handle, int function, struct acpi_buffer *params, struct acpi_buffer *output) { acpi_status status; union acpi_object atif_arg_elements[2]; struct acpi_object_list atif_arg; - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL}; + union acpi_object *obj;
- atif_arg.count = 2; + atif_arg.count = 1; atif_arg.pointer = &atif_arg_elements[0];
atif_arg_elements[0].type = ACPI_TYPE_INTEGER; - atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE; - atif_arg_elements[1].type = ACPI_TYPE_INTEGER; - atif_arg_elements[1].integer.value = 0; + atif_arg_elements[0].integer.value = function;
- status = acpi_evaluate_object(handle, "ATIF", &atif_arg, &buffer); + if (params) { + atif_arg.count = 2; + atif_arg_elements[1].type = ACPI_TYPE_BUFFER; + atif_arg_elements[1].buffer.length = params->length; + atif_arg_elements[1].buffer.pointer = params->pointer; + }
- /* Fail only if calling the method fails and ATIF is supported */ - if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) { - DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n", - acpi_format_exception(status)); - kfree(buffer.pointer); + status = acpi_evaluate_object(handle, "ATIF", &atif_arg, output); + if (ACPI_FAILURE(status)) return 1; + + obj = (union acpi_object *) output->pointer; + if (!obj) + return 1; + else if (obj->type != ACPI_TYPE_BUFFER) { + kfree(obj); + return 1; + } else if (obj->buffer.length != 256) { + DRM_DEBUG_DRIVER("Unknown ATIF output buffer length %d\n", obj->buffer.length); + kfree(obj); + return 1; + } + + return 0; +} + +static int radeon_atif_verify(acpi_handle handle) +{ + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + struct atif_verify_output *voutput; + union acpi_object *obj; + int ret; + + /* evaluate the ATIF verify function */ + ret = radeon_atif_call(handle, ATIF_FUNCTION_VERIFY_INTERFACE, NULL, &output); + if (ret) { + kfree(output.pointer); + return ret; + } + + obj = (union acpi_object *) output.pointer; + + voutput = (struct atif_verify_output *) obj->buffer.pointer; + verify_output = kzalloc(sizeof(struct atif_verify_output), GFP_KERNEL); /* TODO: kfree */ + verify_output->size = voutput->size; + verify_output->version = voutput->version; + verify_output->notif_mask = voutput->notif_mask; + verify_output->func_bits = voutput->func_bits; + + kfree(output.pointer); + return 0; +} + +static int radeon_acpi_video_event(struct notifier_block *nb, + unsigned long val, void *data) +{ + /* The only video events relevant to opregion are 0x81 or n. These indicate + either a docking event, lid switch or display switch request. In + Linux, these are handled by the dock, button and video drivers. + */ + + struct acpi_bus_event *event = data; + int ret = NOTIFY_BAD; /* Question: for fill acpi's blocking notifier chains */ + + if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0) + return NOTIFY_DONE; + + if (event->type != notify_code) + return NOTIFY_DONE; + + /* TODO: run anything should take care by radeon */ + + return ret; +} + +static struct notifier_block radeon_acpi_notifier = { + .notifier_call = radeon_acpi_video_event, +}; + +static int radeon_atif_get_system_param(acpi_handle handle) +{ + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + struct atif_get_sysparams_output *params_output; + union acpi_object *obj; + u32 flags; + int ret; + + /* evaluate the ATIF get system parameters function */ + ret = radeon_atif_call(handle, ATIF_FUNCTION_GET_SYSTEM_PARAMETERS, NULL, &output); + if (ret) { + kfree(output.pointer); + return ret; }
- kfree(buffer.pointer); + obj = (union acpi_object *) output.pointer; + + params_output = (struct atif_get_sysparams_output *) obj->buffer.pointer; + flags = params_output->flags; + + if (flags) { + if (flags == 1) + notify_code = 0x81; + else if (flags == 2) + notify_code = params_output->notify_code; + + register_acpi_notifier(&radeon_acpi_notifier); + } + + kfree(output.pointer); return 0; }
@@ -59,11 +174,16 @@ int radeon_acpi_init(struct radeon_device *rdev) if (!ASIC_IS_AVIVO(rdev) || !rdev->bios || !handle) return 0;
- /* Call the ATIF method */ - ret = radeon_atif_call(handle); + /* Call the ATIF verify function */ + ret = radeon_atif_verify(handle); if (ret) return ret;
+ if (verify_output->func_bits & ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED) + ret = radeon_atif_get_system_param(handle); + if (ret) + return ret; + return 0; }
On Mon, Jul 30, 2012 at 04:32:47PM +0800, joeyli wrote:
於 日，2012-07-29 於 15:10 +0200，Luca Tettamanti 提到：
On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
Hi Luca,
於 六，2012-07-28 於 16:56 +0200，Luca Tettamanti 提到：
I just found the first problem (probably a BIOS bug): ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :( I intended to use the method to set up the notification handler but now my BIOS says that it's not there even if it is... Can I assume some default values (e.g. notifications are enabled and will use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something different)?
Did you check your DSDT for there have some "Notify (VGA, 0x81)" statement in AFN0..AFN15? If YES, I think that means your machine in case the 0x81 is for ATI used by default.
Yes, my point is that the nofication is there, but since GET_SYSTEM_PARAMETERS is not announced I have not way to check it. IOW, what is implemented in the DSDT does not match what is announced by VERIFY_INTERFACE. For reference this is the DSDT: http://pastebin.com/KKS7ZsTt
Luca
Yes, saw the problem in your DSDT:
Method (AF00, 0, NotSerialized) { CreateWordField (ATIB, Zero, SSZE) ... Store (0x80, NMSK) Store (0x02, SFUN) <=== 10b, bit 0 is 0 Return (ATIB) }But, AF01 still supported in ATIF on this machine, maybe we should still try GET_SYSTEM_PARAMETERS even the function vector set to 0? No idea...
That's what I'm doing right now... if SBIOS_REQUESTS is supported I try and call GET_SYSTEM_PARAMETERS even if it's not announced.
On the other hand, My patch to avoid 0x81 event conflict with acpi/video driver like below. This patch woks on my notebook. Your patches do much more things then mine, so I think my patch just for reference.
I ignored the event handling for now... I'd like to hear something back from ACPI camp before committing to this solution.
There have a problem is: If we want use acpi_notifier_call_chain to check does event consume by any notifier in acpi's blocking notifier chain, then we need return NOTIFY_BAD in radeon_acpi but not NOTIFY_OK.
So, I suggest radeon_acpi should register to acpi notifier chain by itself but not append on radeon_acpi_event in radeon_pm.
It shouldn't matter, once I change radeon_atif_handler to return NOTIFY_BAD the call chain will be stopped anyway.
And, suggest also check the device class is ACPI_VIDEO_CLASS like following:
+static int radeon_acpi_video_event(struct notifier_block *nb, ...
- if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
return NOTIFY_DONE;
Will do. I'll use the package structs in the next iteration.
Luca
於 四，2012-07-26 於 14:58 +0200，Luca Tettamanti 提到：
- again ACPI video module gets the nodification (in this case ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the
hypothetical radeon driver does its magic to adjust the screen.
My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the acpi notifier chain, and allow the handlers to veto the key press (like it's done for ACPI_VIDEO_NOTIFY_SWITCH).
I welcome this approach!
On some ATI machine's DSDT also issue ACPI_VIDEO_NOTIFY_PROBE when AC-power unplug, because BIOS want to nodify video driver do some power saving stuff. It causes KEY_SWITCHVIDEOMODE issued by acpi/video driver when AC-power unplug.
At least acpi/video driver need to know this 0x81 event is filed by BIOS to radeon-acpi for notify but not do video mode switch. That means the radeon drm need take the video switch responsibility.
Thanks a lot! Joey Lee
On Thu, Jul 26, 2012 at 10:50 PM, joeyli jlee@suse.com wrote:
於 四，2012-07-26 於 14:58 +0200，Luca Tettamanti 提到：
- again ACPI video module gets the nodification (in this case ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the
hypothetical radeon driver does its magic to adjust the screen.
My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the acpi notifier chain, and allow the handlers to veto the key press (like it's done for ACPI_VIDEO_NOTIFY_SWITCH).
I welcome this approach!
On some ATI machine's DSDT also issue ACPI_VIDEO_NOTIFY_PROBE when AC-power unplug, because BIOS want to nodify video driver do some power saving stuff. It causes KEY_SWITCHVIDEOMODE issued by acpi/video driver when AC-power unplug.
At least acpi/video driver need to know this 0x81 event is filed by BIOS to radeon-acpi for notify but not do video mode switch. That means the radeon drm need take the video switch responsibility.
Probably we'd just want the radeon acpi handler to just forward the events to userspace so that the user can choose what to do with it (xrandr command, etc.), otherwise we'll need to define policy in the driver.
Alex
Thanks a lot! Joey Lee
於 四，2012-07-26 於 23:31 -0400，Alex Deucher 提到：
On Thu, Jul 26, 2012 at 10:50 PM, joeyli jlee@suse.com wrote:
於 四，2012-07-26 於 14:58 +0200，Luca Tettamanti 提到：
- again ACPI video module gets the nodification (in this case ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the
hypothetical radeon driver does its magic to adjust the screen.
My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the acpi notifier chain, and allow the handlers to veto the key press (like it's done for ACPI_VIDEO_NOTIFY_SWITCH).
I welcome this approach!
On some ATI machine's DSDT also issue ACPI_VIDEO_NOTIFY_PROBE when AC-power unplug, because BIOS want to nodify video driver do some power saving stuff. It causes KEY_SWITCHVIDEOMODE issued by acpi/video driver when AC-power unplug.
At least acpi/video driver need to know this 0x81 event is filed by BIOS to radeon-acpi for notify but not do video mode switch. That means the radeon drm need take the video switch responsibility.
Probably we'd just want the radeon acpi handler to just forward the events to userspace so that the user can choose what to do with it (xrandr command, etc.), otherwise we'll need to define policy in the driver.
Alex
Any kernel module can issue KEY_SWITCHVIDEOMODE to user space, then gnome-settings-daemon(on Gnome) and krandr(on KDE) will call xrandr library to switch video mode.
The 0x81 ACPI event for acpi/video driver is ACPI_VIDEO_NOTIFY_PROBE, means need issue KEY_SWITCHVIDEOMODE. But, 0x81 for radeon is a general notification event. I didn't see probe state in GET_SYSTEM_BIOS_REQUESTS, how can we distinguish this 0x81 is a ACPI_VIDEO_NOTIFY_PROBE or a ATI general notification event?
+ * flags + * bits 1:0: + * 0 - Notify(VGA, 0x81) is not used for notification + * 1 - Notify(VGA, 0x81) is used for notification
Per the above flags, when we detect bit set to 1, means 0x81 used for radeon-acpi to be a general notification event. My question is: what's the event number for ACPI_VIDEO_NOTIFY_PROBE on this AMD/ATI machine when 0x81 not available for acpi/video?
Thanks a lot! Joey Lee
On Fri, Jul 27, 2012 at 12:46:50PM +0800, joeyli wrote:
於 四，2012-07-26 於 23:31 -0400，Alex Deucher 提到：
On Thu, Jul 26, 2012 at 10:50 PM, joeyli jlee@suse.com wrote:
於 四，2012-07-26 於 14:58 +0200，Luca Tettamanti 提到：
- again ACPI video module gets the nodification (in this case ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the
hypothetical radeon driver does its magic to adjust the screen.
My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the acpi notifier chain, and allow the handlers to veto the key press (like it's done for ACPI_VIDEO_NOTIFY_SWITCH).
I welcome this approach!
On some ATI machine's DSDT also issue ACPI_VIDEO_NOTIFY_PROBE when AC-power unplug, because BIOS want to nodify video driver do some power saving stuff. It causes KEY_SWITCHVIDEOMODE issued by acpi/video driver when AC-power unplug.
At least acpi/video driver need to know this 0x81 event is filed by BIOS to radeon-acpi for notify but not do video mode switch. That means the radeon drm need take the video switch responsibility.
Probably we'd just want the radeon acpi handler to just forward the events to userspace so that the user can choose what to do with it (xrandr command, etc.), otherwise we'll need to define policy in the driver.
Any kernel module can issue KEY_SWITCHVIDEOMODE to user space, then gnome-settings-daemon(on Gnome) and krandr(on KDE) will call xrandr library to switch video mode.
Exacly, and if we have pending system bios requests then the event is not a real ACPI_VIDEO_NOTIFY_PROBE and (IMHO) it should not be forwarded to the userspace as such. The "vanilla" ACPI_VIDEO_NOTIFY_PROBE is already forwared to userspace.
The 0x81 ACPI event for acpi/video driver is ACPI_VIDEO_NOTIFY_PROBE, means need issue KEY_SWITCHVIDEOMODE. But, 0x81 for radeon is a general notification event. I didn't see probe state in GET_SYSTEM_BIOS_REQUESTS, how can we distinguish this 0x81 is a ACPI_VIDEO_NOTIFY_PROBE or a ATI general notification event?
+#define ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS 0x2 +/* ARG0: ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * DWORD - pending sbios requests + * BYTE - panel expansion mode + * BYTE - thermal state: target gfx controller + * BYTE - thermal state: state id (0: exit state, non-0: state) + * BYTE - forced power state: target gfx controller + * BYTE - forced power state: state id + * BYTE - system power source + * BYTE - panel backlight level (0-255)
I guess that if "pending sbios requests" == 0 then the event is the general purpose one, and is not handled by radeon driver.
Luca
On Fri, Jul 27, 2012 at 12:46 AM, joeyli jlee@suse.com wrote:
於 四，2012-07-26 於 23:31 -0400，Alex Deucher 提到：
On Thu, Jul 26, 2012 at 10:50 PM, joeyli jlee@suse.com wrote:
於 四，2012-07-26 於 14:58 +0200，Luca Tettamanti 提到：
- again ACPI video module gets the nodification (in this case ACPI_VIDEO_NOTIFY_PROBE), re-enumerated and send KEY_SWITCHVIDEOMODE
- KDE seems this and muck with the screen configuration :(
- meanwhile the brightness notification is propagated, the
hypothetical radeon driver does its magic to adjust the screen.
My first idea would be to make ACPI_VIDEO_NOTIFY_PROBE also call to the acpi notifier chain, and allow the handlers to veto the key press (like it's done for ACPI_VIDEO_NOTIFY_SWITCH).
I welcome this approach!
On some ATI machine's DSDT also issue ACPI_VIDEO_NOTIFY_PROBE when AC-power unplug, because BIOS want to nodify video driver do some power saving stuff. It causes KEY_SWITCHVIDEOMODE issued by acpi/video driver when AC-power unplug.
At least acpi/video driver need to know this 0x81 event is filed by BIOS to radeon-acpi for notify but not do video mode switch. That means the radeon drm need take the video switch responsibility.
Probably we'd just want the radeon acpi handler to just forward the events to userspace so that the user can choose what to do with it (xrandr command, etc.), otherwise we'll need to define policy in the driver.
Alex
Any kernel module can issue KEY_SWITCHVIDEOMODE to user space, then gnome-settings-daemon(on Gnome) and krandr(on KDE) will call xrandr library to switch video mode.
The 0x81 ACPI event for acpi/video driver is ACPI_VIDEO_NOTIFY_PROBE, means need issue KEY_SWITCHVIDEOMODE. But, 0x81 for radeon is a general notification event. I didn't see probe state in GET_SYSTEM_BIOS_REQUESTS, how can we distinguish this 0x81 is a ACPI_VIDEO_NOTIFY_PROBE or a ATI general notification event?
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
Per the above flags, when we detect bit set to 1, means 0x81 used for radeon-acpi to be a general notification event. My question is: what's the event number for ACPI_VIDEO_NOTIFY_PROBE on this AMD/ATI machine when 0x81 not available for acpi/video?
+/* ARG0: ATIF_FUNCTION_GET_SYSTEM_PARAMETERS + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * DWORD - valid flags mask + * DWORD - flags + * + * OR + * + * WORD - structure size in bytes (includes size field) + * DWORD - valid flags mask + * DWORD - flags + * BYTE - notify command code + * + * flags + * bits 1:0: + * 0 - Notify(VGA, 0x81) is not used for notification + * 1 - Notify(VGA, 0x81) is used for notification + * 2 - Notify(VGA, n) is used for notification where + * n (0xd0-0xd9) is specified in notify command code. + * bit 2: + * 1 - lid changes not reported though int10 + */
if bits 1:0 == 0, there is no notify event for radeon. When bits 1:0 == 1, it uses 0x81; when bits 1:0 == 2 it uses the event number specified in the following byte (notify command code) which would be something in the 0xd0-0xd9 range.
Alex
於 五，2012-07-27 於 09:21 -0400，Alex Deucher 提到：
On Fri, Jul 27, 2012 at 12:46 AM, joeyli jlee@suse.com wrote:
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
Per the above flags, when we detect bit set to 1, means 0x81 used for radeon-acpi to be a general notification event. My question is: what's the event number for ACPI_VIDEO_NOTIFY_PROBE on this AMD/ATI machine when 0x81 not available for acpi/video?
+/* ARG0: ATIF_FUNCTION_GET_SYSTEM_PARAMETERS
- ARG1: none
- OUTPUT:
- WORD - structure size in bytes (includes size field)
- DWORD - valid flags mask
- DWORD - flags
- OR
- WORD - structure size in bytes (includes size field)
- DWORD - valid flags mask
- DWORD - flags
- BYTE - notify command code
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
- 2 - Notify(VGA, n) is used for notification where
- n (0xd0-0xd9) is specified in notify command code.
- bit 2:
- 1 - lid changes not reported though int10
- */
if bits 1:0 == 0, there is no notify event for radeon. When bits 1:0 == 1, it uses 0x81; when bits 1:0 == 2 it uses the event number specified in the following byte (notify command code) which would be something in the 0xd0-0xd9 range.
Alex
Did you mean every time we received 0x81 event in kernel module, we need access GET_SYSTEM_PARAMETERS to get the flags for distinguish between ACPI_VIDEO_NOTIFY_PROBE?
Or just need access ONE time when system boot?
I have a machine the GET_SYSTEM_PARAMETERS looks like this:
Method (AF01, 0, NotSerialized) /* ATIF_FUNCTION_GET_SYSTEM_PARAMETERS 0x1 */ { CreateWordField (ATIB, Zero, SSZE) CreateDWordField (ATIB, 0x02, VMSK) CreateDWordField (ATIB, 0x06, FLGS) /* flags bits 1:0 */ Store (0x0A, SSZE) /* structure SIZE fixed */ Store (0x03, VMSK) /* valid flags mask fixed to 0x03 */ Store (One, FLGS) /* FLAGS always set to 1 */ Return (ATIB) }
Looks like just need access ONE time when system boot because those return value of AF01 fixed in DSDT.
On this machine doesn't support probe event, I didn't see any event issued when I plug D-Sub. Does that means those kind of ATI/AMD machines do NOT support probe notify?
If YES, then we can just direct disable acpi/video driver by radeon-acpi when we detected FLAGS is 1.
Thanks a lot! Joey Lee
於 五，2012-07-27 於 23:32 +0800，joeyli 提到：
於 五，2012-07-27 於 09:21 -0400，Alex Deucher 提到：
On Fri, Jul 27, 2012 at 12:46 AM, joeyli jlee@suse.com wrote:
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
Per the above flags, when we detect bit set to 1, means 0x81 used for radeon-acpi to be a general notification event. My question is: what's the event number for ACPI_VIDEO_NOTIFY_PROBE on this AMD/ATI machine when 0x81 not available for acpi/video?
+/* ARG0: ATIF_FUNCTION_GET_SYSTEM_PARAMETERS
- ARG1: none
- OUTPUT:
- WORD - structure size in bytes (includes size field)
- DWORD - valid flags mask
- DWORD - flags
- OR
- WORD - structure size in bytes (includes size field)
- DWORD - valid flags mask
- DWORD - flags
- BYTE - notify command code
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
- 2 - Notify(VGA, n) is used for notification where
- n (0xd0-0xd9) is specified in notify command code.
- bit 2:
- 1 - lid changes not reported though int10
- */
if bits 1:0 == 0, there is no notify event for radeon. When bits 1:0 == 1, it uses 0x81; when bits 1:0 == 2 it uses the event number specified in the following byte (notify command code) which would be something in the 0xd0-0xd9 range.
Alex
Did you mean every time we received 0x81 event in kernel module, we need access GET_SYSTEM_PARAMETERS to get the flags for distinguish between ACPI_VIDEO_NOTIFY_PROBE?
Or just need access ONE time when system boot?
I have a machine the GET_SYSTEM_PARAMETERS looks like this:
Method (AF01, 0, NotSerialized) /* ATIF_FUNCTION_GET_SYSTEM_PARAMETERS 0x1 */ { CreateWordField (ATIB, Zero, SSZE) CreateDWordField (ATIB, 0x02, VMSK) CreateDWordField (ATIB, 0x06, FLGS) /* flags bits 1:0 */ Store (0x0A, SSZE) /* structure SIZE fixed */ Store (0x03, VMSK) /* valid flags mask fixed to 0x03 */ Store (One, FLGS) /* FLAGS always set to 1 */ Return (ATIB) }
Looks like just need access ONE time when system boot because those return value of AF01 fixed in DSDT.
On this machine doesn't support probe event, I didn't see any event issued when I plug D-Sub. Does that means those kind of ATI/AMD machines do NOT support probe notify?
If YES, then we can just direct disable acpi/video driver by radeon-acpi
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
when we detected FLAGS is 1.
I mean disable the ACPI_VIDEO_NOTIFY_PROBE related code in acpi/video driver.
Thanks Joey Lee
On Fri, Jul 27, 2012 at 11:32 AM, joeyli jlee@suse.com wrote:
於 五，2012-07-27 於 09:21 -0400，Alex Deucher 提到：
On Fri, Jul 27, 2012 at 12:46 AM, joeyli jlee@suse.com wrote:
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
Per the above flags, when we detect bit set to 1, means 0x81 used for radeon-acpi to be a general notification event. My question is: what's the event number for ACPI_VIDEO_NOTIFY_PROBE on this AMD/ATI machine when 0x81 not available for acpi/video?
+/* ARG0: ATIF_FUNCTION_GET_SYSTEM_PARAMETERS
- ARG1: none
- OUTPUT:
- WORD - structure size in bytes (includes size field)
- DWORD - valid flags mask
- DWORD - flags
- OR
- WORD - structure size in bytes (includes size field)
- DWORD - valid flags mask
- DWORD - flags
- BYTE - notify command code
- flags
- bits 1:0:
- 0 - Notify(VGA, 0x81) is not used for notification
- 1 - Notify(VGA, 0x81) is used for notification
- 2 - Notify(VGA, n) is used for notification where
- n (0xd0-0xd9) is specified in notify command code.
- bit 2:
- 1 - lid changes not reported though int10
- */
if bits 1:0 == 0, there is no notify event for radeon. When bits 1:0 == 1, it uses 0x81; when bits 1:0 == 2 it uses the event number specified in the following byte (notify command code) which would be something in the 0xd0-0xd9 range.
Alex
Did you mean every time we received 0x81 event in kernel module, we need access GET_SYSTEM_PARAMETERS to get the flags for distinguish between ACPI_VIDEO_NOTIFY_PROBE?
Or just need access ONE time when system boot?
Just once at start up to see what event, if any, the sbios will use to signal the driver.
I have a machine the GET_SYSTEM_PARAMETERS looks like this:
Method (AF01, 0, NotSerialized) /* ATIF_FUNCTION_GET_SYSTEM_PARAMETERS 0x1 */ { CreateWordField (ATIB, Zero, SSZE) CreateDWordField (ATIB, 0x02, VMSK) CreateDWordField (ATIB, 0x06, FLGS) /* flags bits 1:0 */ Store (0x0A, SSZE) /* structure SIZE fixed */ Store (0x03, VMSK) /* valid flags mask fixed to 0x03 */ Store (One, FLGS) /* FLAGS always set to 1 */ Return (ATIB) }
Looks like just need access ONE time when system boot because those return value of AF01 fixed in DSDT.
On this machine doesn't support probe event, I didn't see any event issued when I plug D-Sub. Does that means those kind of ATI/AMD machines do NOT support probe notify?
Analog connectors don't support hotplug at all. And hotplug on digital connectors is already handled by the driver. As far as I know ACPI doesn't provide any events for these. The driver gets interrupts directly from the hw. The only events the sbios uses the notify event for are the ones listed in the supported notifications mask from ATIF_FUNCTION_VERIFY_INTERFACE:
+#define ATIF_FUNCTION_VERIFY_INTERFACE 0x0 +/* ARG0: ATIF_FUNCTION_VERIFY_INTERFACE + * ARG1: none + * OUTPUT: + * WORD - structure size in bytes (includes size field) + * WORD - version + * DWORD - supported notifications mask + * DWORD - supported functions bit vector + */ +/* Notifications mask */ +# define ATIF_DISPLAY_SWITCH_REQUEST_SUPPORTED (1 << 0) +# define ATIF_EXPANSION_MODE_CHANGE_REQUEST_SUPPORTED (1 << 1) +# define ATIF_THERMAL_STATE_CHANGE_REQUEST_SUPPORTED (1 << 2) +# define ATIF_FORCED_POWER_STATE_CHANGE_REQUEST_SUPPORTED (1 << 3) +# define ATIF_SYSTEM_POWER_SOURCE_CHANGE_REQUEST_SUPPORTED (1 << 4) +# define ATIF_DISPLAY_CONF_CHANGE_REQUEST_SUPPORTED (1 << 5) +# define ATIF_PX_GFX_SWITCH_REQUEST_SUPPORTED (1 << 6) +# define ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST_SUPPORTED (1 << 7) +# define ATIF_DGPU_DISPLAY_EVENT_SUPPORTED (1 << 8)
They are things like the user presses the display switch hotkey or presses the brightness up hotkey.
Alex
dri-devel@lists.freedesktop.org
-
Alex Deucher -
alexdeucher＠gmail.com
-
joeyli -
Luca Tettamanti -
Zhang Rui