Re: [PATCH v4] drm/amdgpu: Disable ABM when AC mode
From: Harry Wentland
Date: Wed Apr 27 2022 - 11:22:06 EST
On 2022-04-27 04:08, Ryan Lin wrote:
Disable ABM feature when the system is running on AC mode to get the more
perfect contrast of the display.
v2: remove "UPSTREAM" from the subject.
v3: adv->pm.ac_power updating by amd gpu_acpi_event_handler.
V4: Add the file I lost to fix the build error.
Signed-off-by: Ryan Lin <tsung-hua.lin@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 3 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 61 +++++++++++--------
drivers/gpu/drm/amd/include/amd_acpi.h | 1 +
drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 1 +
5 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 4811b0faafd9a..6ac331ee4255d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -822,7 +822,8 @@ static int amdgpu_acpi_event(struct notifier_block *nb,
struct amdgpu_device *adev = container_of(nb, struct amdgpu_device, acpi_nb);
struct acpi_bus_event *entry = (struct acpi_bus_event *)data;
- if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0) {
+ if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0 ||
+ strcmp(entry->device_class, ACPI_BATTERY_CLASS) == 0) {
if (power_supply_is_system_supplied() > 0)
DRM_DEBUG_DRIVER("pm: AC\n");
else
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index abfcc1304ba0c..3a0afe7602727 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3454,6 +3454,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
adev->gfx.gfx_off_req_count = 1;
adev->pm.ac_power = power_supply_is_system_supplied() > 0;
+ adev->pm.old_ac_power = true;
atomic_set(&adev->throttling_logging_enabled, 1);
/*
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
index 54a1408c8015c..090bd23410b45 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
@@ -22,7 +22,8 @@
* Authors: AMD
*
*/
-
+#include <linux/power_supply.h>
+#include "amdgpu.h"
#include "dmub_abm.h"
#include "dce_abm.h"
#include "dc.h"
@@ -50,7 +51,7 @@
#define DISABLE_ABM_IMMEDIATELY 255
-
+extern uint amdgpu_dm_abm_level;
static void dmub_abm_enable_fractional_pwm(struct dc_context *dc)
{
@@ -117,28 +118,6 @@ static void dmub_abm_init(struct abm *abm, uint32_t backlight)
dmub_abm_enable_fractional_pwm(abm->ctx);
}
-static unsigned int dmub_abm_get_current_backlight(struct abm *abm)
-{
- struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
- unsigned int backlight = REG_READ(BL1_PWM_CURRENT_ABM_LEVEL);
-
- /* return backlight in hardware format which is unsigned 17 bits, with
- * 1 bit integer and 16 bit fractional
- */
- return backlight;
-}
-
-static unsigned int dmub_abm_get_target_backlight(struct abm *abm)
-{
- struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
- unsigned int backlight = REG_READ(BL1_PWM_TARGET_ABM_LEVEL);
-
- /* return backlight in hardware format which is unsigned 17 bits, with
- * 1 bit integer and 16 bit fractional
- */
- return backlight;
-}
-
static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
{
union dmub_rb_cmd cmd;
@@ -147,6 +126,10 @@ static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
int i;
int edp_num;
uint8_t panel_mask = 0;
+ struct amdgpu_device *dev = dc->driver_context;
NAK. We can't access amdgpu_device in DC. This is code that's
shared with other OSes.
I've mentioned this in my previous review a month ago.
What happened to the other suggestion I had? I never saw
a follow-up.
My previous comments, copy-pasted here again. Please address
or answer why you disagree:
This patch still has the problem of accessing adev from within DC.
That'll break things on other platforms. This information needs to
come in through the DC interface if we want to enable/disable ABM in
this function.
After a closer look I also don't think that amdgpu is the right place
to control the logic to disable ABM in AC mode, i.e. to switch between
ABM levels. Take a look at dm_connector_state.abm_level and the
abm_level_property. It's exposed to userspace as "abm level".
The "abm level" defaults to "0" unless userspace sets the "abm level"
to something else. The same component that sets the "abm level"
initially is the one that should set it to "0" when in AC mode.
It might be that the ABM level is controlled via the abmlevel module
parameter. If that's the case and there isn't a userspace that sets the
"abm level" property then the easiest way to handle this is to switch
between 0 and amdgpu_dm_abm_level inside amdgpu_dm when there is a
AC/DC switch. Either way we shouldn't need to change DC.
Thanks,
Harry
+
+ if (dev->pm.ac_power)
+ level = 0;
get_edp_links(dc->dc, edp_links, &edp_num);
@@ -170,6 +153,36 @@ static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
return true;
}
+static unsigned int dmub_abm_get_current_backlight(struct abm *abm)
+{
+ struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
+ unsigned int backlight = REG_READ(BL1_PWM_CURRENT_ABM_LEVEL);
+ struct dc_context *dc = abm->ctx;
+ struct amdgpu_device *adev = dc->driver_context;
+
+ if (adev->pm.ac_power != adev->pm.old_ac_power) {
+ dmub_abm_set_level(abm, amdgpu_dm_abm_level);
+ adev->pm.ac_power = power_supply_is_system_supplied() > 0;
+ adev->pm.old_ac_power = adev->pm.ac_power;
+ }
+
+ /* return backlight in hardware format which is unsigned 17 bits, with
+ * 1 bit integer and 16 bit fractional
+ */
+ return backlight;
+}
+
+static unsigned int dmub_abm_get_target_backlight(struct abm *abm)
+{
+ struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
+ unsigned int backlight = REG_READ(BL1_PWM_TARGET_ABM_LEVEL);
+
+ /* return backlight in hardware format which is unsigned 17 bits, with
+ * 1 bit integer and 16 bit fractional
+ */
+ return backlight;
+}
+
static bool dmub_abm_init_config(struct abm *abm,
const char *src,
unsigned int bytes,
diff --git a/drivers/gpu/drm/amd/include/amd_acpi.h b/drivers/gpu/drm/amd/include/amd_acpi.h
index 2d089d30518f3..5a62e580668e4 100644
--- a/drivers/gpu/drm/amd/include/amd_acpi.h
+++ b/drivers/gpu/drm/amd/include/amd_acpi.h
@@ -25,6 +25,7 @@
#define AMD_ACPI_H
#define ACPI_AC_CLASS "ac_adapter"
+#define ACPI_BATTERY_CLASS "battery"
struct atif_verify_interface {
u16 size; /* structure size in bytes (includes size field) */
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
index f6e0e7d8a0077..de459411a0e83 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
@@ -445,6 +445,7 @@ struct amdgpu_pm {
uint32_t smu_prv_buffer_size;
struct amdgpu_bo *smu_prv_buffer;
bool ac_power;
+ bool old_ac_power;
/* powerplay feature */
uint32_t pp_feature;