Disable ABM feature when the system is running on AC mode to get
the more perfect contrast of the display.
Signed-off-by: Ryan Lin <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 58 ++++++++++++-------
drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 1 +
4 files changed, 42 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index c560c1ab62ecb..bc8bb9aad2e36 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -822,6 +822,10 @@ 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, "battery") == 0) {
+ adev->pm.ac_power = power_supply_is_system_supplied() > 0;
+ }
+
if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0) {
if (power_supply_is_system_supplied() > 0)
DRM_DEBUG_DRIVER("pm: AC\n");
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..478a734b66926 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
@@ -23,6 +23,8 @@
*
*/
+#include <linux/power_supply.h>
+#include "amdgpu.h"
#include "dmub_abm.h"
#include "dce_abm.h"
#include "dc.h"
@@ -51,6 +53,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 +120,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;
@@ -148,6 +129,9 @@ static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
int edp_num;
uint8_t panel_mask = 0;
+ if (power_supply_is_system_supplied() > 0)
+ level = 0;
+
get_edp_links(dc->dc, edp_links, &edp_num);
for (i = 0; i < edp_num; i++) {
@@ -170,6 +154,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/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;
--
2.25.1
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.
Signed-off-by: Ryan Lin <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 58 ++++++++++++-------
drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 1 +
4 files changed, 42 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index c560c1ab62ecb..bc8bb9aad2e36 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -822,6 +822,10 @@ 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, "battery") == 0) {
+ adev->pm.ac_power = power_supply_is_system_supplied() > 0;
+ }
+
if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0) {
if (power_supply_is_system_supplied() > 0)
DRM_DEBUG_DRIVER("pm: AC\n");
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..478a734b66926 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
@@ -23,6 +23,8 @@
*
*/
+#include <linux/power_supply.h>
+#include "amdgpu.h"
#include "dmub_abm.h"
#include "dce_abm.h"
#include "dc.h"
@@ -51,6 +53,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 +120,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;
@@ -148,6 +129,9 @@ static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
int edp_num;
uint8_t panel_mask = 0;
+ if (power_supply_is_system_supplied() > 0)
+ level = 0;
+
get_edp_links(dc->dc, edp_links, &edp_num);
for (i = 0; i < edp_num; i++) {
@@ -170,6 +154,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/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;
--
2.25.1
On 2022-03-29 10:33, Harry Wentland wrote:
>
>
> On 2022-03-24 19:10, Ryan Lin wrote:
>> Disable ABM feature when the system is running on AC mode to get
>> the more perfect contrast of the display.
>>
>> Signed-off-by: Ryan Lin <[email protected]>
>>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
>> drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 58 ++++++++++++-------
>> drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 1 +
>> 4 files changed, 42 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> index c560c1ab62ecb..bc8bb9aad2e36 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> @@ -822,6 +822,10 @@ 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, "battery") == 0) {
>> + adev->pm.ac_power = power_supply_is_system_supplied() > 0;
>> + }
>> +
>> if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0) {
>> if (power_supply_is_system_supplied() > 0)
>> DRM_DEBUG_DRIVER("pm: AC\n");
>> 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..478a734b66926 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
>> @@ -23,6 +23,8 @@
>> *
>> */
>>
>> +#include <linux/power_supply.h>
>> +#include "amdgpu.h"
>> #include "dmub_abm.h"
>> #include "dce_abm.h"
>> #include "dc.h"
>> @@ -51,6 +53,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 +120,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;
>> @@ -148,6 +129,9 @@ static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
>> int edp_num;
>> uint8_t panel_mask = 0;
>>
>> + if (power_supply_is_system_supplied() > 0)
>> + level = 0;
>> +
>> get_edp_links(dc->dc, edp_links, &edp_num);
>>
>> for (i = 0; i < edp_num; i++) {
>> @@ -170,6 +154,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) {
>
> 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.
Harry
> Harry
>
>> + 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/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;
>>
>
On 2022-03-24 19:10, Ryan Lin wrote:
> Disable ABM feature when the system is running on AC mode to get
> the more perfect contrast of the display.
>
> Signed-off-by: Ryan Lin <[email protected]>
>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
> drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 58 ++++++++++++-------
> drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 1 +
> 4 files changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index c560c1ab62ecb..bc8bb9aad2e36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -822,6 +822,10 @@ 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, "battery") == 0) {
> + adev->pm.ac_power = power_supply_is_system_supplied() > 0;
> + }
> +
> if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0) {
> if (power_supply_is_system_supplied() > 0)
> DRM_DEBUG_DRIVER("pm: AC\n");
> 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..478a734b66926 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
> @@ -23,6 +23,8 @@
> *
> */
>
> +#include <linux/power_supply.h>
> +#include "amdgpu.h"
> #include "dmub_abm.h"
> #include "dce_abm.h"
> #include "dc.h"
> @@ -51,6 +53,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 +120,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;
> @@ -148,6 +129,9 @@ static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
> int edp_num;
> uint8_t panel_mask = 0;
>
> + if (power_supply_is_system_supplied() > 0)
> + level = 0;
> +
> get_edp_links(dc->dc, edp_links, &edp_num);
>
> for (i = 0; i < edp_num; i++) {
> @@ -170,6 +154,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) {
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.
Harry
> + 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/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;
>
On Tue, Mar 29, 2022 at 4:56 AM Ryan Lin <[email protected]> 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.
>
> Signed-off-by: Ryan Lin <[email protected]>
>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
> drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 58 ++++++++++++-------
> drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 1 +
> 4 files changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index c560c1ab62ecb..bc8bb9aad2e36 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -822,6 +822,10 @@ 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, "battery") == 0) {
> + adev->pm.ac_power = power_supply_is_system_supplied() > 0;
> + }
> +
Is this change necessary? As I said before, we already update
adev->pm.ac_power a few lines later in amdgpu_pm_acpi_event_handler().
If there is something wrong with that code, please adjust as
necessary.
Alex
> if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0) {
> if (power_supply_is_system_supplied() > 0)
> DRM_DEBUG_DRIVER("pm: AC\n");
> 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..478a734b66926 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
> @@ -23,6 +23,8 @@
> *
> */
>
> +#include <linux/power_supply.h>
> +#include "amdgpu.h"
> #include "dmub_abm.h"
> #include "dce_abm.h"
> #include "dc.h"
> @@ -51,6 +53,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 +120,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;
> @@ -148,6 +129,9 @@ static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
> int edp_num;
> uint8_t panel_mask = 0;
>
> + if (power_supply_is_system_supplied() > 0)
> + level = 0;
> +
> get_edp_links(dc->dc, edp_links, &edp_num);
>
> for (i = 0; i < edp_num; i++) {
> @@ -170,6 +154,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/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;
>
> --
> 2.25.1
>
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 <[email protected]>
---
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;
+
+ 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;
--
2.25.1
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.
Signed-off-by: Ryan Lin <[email protected]>
---
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 +
4 files changed, 41 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;
+
+ 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) */
--
2.25.1
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 <[email protected]>
>
> ---
> 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;
>
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.
v5: Move that function of the setting abm disabled from DC to amdgpu_dm.
Signed-off-by: Ryan Lin <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 3 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 +++++++++++++++++
drivers/gpu/drm/amd/display/dc/core/dc_link.c | 10 ++++++++++
drivers/gpu/drm/amd/display/dc/dc_link.h | 2 ++
drivers/gpu/drm/amd/include/amd_acpi.h | 1 +
drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 1 +
7 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 4811b0faafd9..6ac331ee4255 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 abfcc1304ba0..b959d256ce46 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 = false;
atomic_set(&adev->throttling_logging_enabled, 1);
/*
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 87283e2da8c1..1ed1f2d00350 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3652,6 +3652,11 @@ amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm)
}
#endif
+static void amdgpu_dm_abm_set_level(struct amdgpu_display_manager *dm, int level)
+{
+ dc_link_set_abm_level(dm->backlight_link[0], level);
+}
+
static int initialize_plane(struct amdgpu_display_manager *dm,
struct amdgpu_mode_info *mode_info, int plane_id,
enum drm_plane_type plane_type,
@@ -9072,6 +9077,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
hdr_changed =
!drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state);
+ if (adev->pm.ac_power)
+ dm_new_crtc_state->abm_level = 0;
+
if (!scaling_changed && !abm_changed && !hdr_changed)
continue;
@@ -9220,6 +9228,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
}
#endif
+
+ if (adev->pm.ac_power != adev->pm.old_ac_power) {
+ if (adev->pm.ac_power)
+ amdgpu_dm_abm_set_level(dm, 0);
+ else
+ amdgpu_dm_abm_set_level(dm, amdgpu_dm_abm_level);
+ adev->pm.old_ac_power = adev->pm.ac_power;
+ }
+
/*
* send vblank event on all events not handled in flip and
* mark consumed event for drm_atomic_helper_commit_hw_done
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index fb012ecd23a1..5edcf2a9dc4e 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -2616,6 +2616,16 @@ int dc_link_get_backlight_level(const struct dc_link *link)
return DC_ERROR_UNEXPECTED;
}
+int dc_link_set_abm_level(const struct dc_link *link, int level)
+{
+ struct abm *abm = get_abm_from_stream_res(link);
+
+ if (abm != NULL && abm->funcs->set_abm_level != NULL)
+ return (int) abm->funcs->set_abm_level(abm, level);
+ else
+ return DC_ERROR_UNEXPECTED;
+}
+
int dc_link_get_target_backlight_pwm(const struct dc_link *link)
{
struct abm *abm = get_abm_from_stream_res(link);
diff --git a/drivers/gpu/drm/amd/display/dc/dc_link.h b/drivers/gpu/drm/amd/display/dc/dc_link.h
index 83845d006c54..b69a114ce154 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_link.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_link.h
@@ -258,6 +258,8 @@ bool dc_link_set_default_brightness_aux(struct dc_link *link);
int dc_link_get_backlight_level(const struct dc_link *dc_link);
+int dc_link_set_abm_level(const struct dc_link *link, int level);
+
int dc_link_get_target_backlight_pwm(const struct dc_link *link);
bool dc_link_set_psr_allow_active(struct dc_link *dc_link, bool enable,
diff --git a/drivers/gpu/drm/amd/include/amd_acpi.h b/drivers/gpu/drm/amd/include/amd_acpi.h
index 2d089d30518f..2d9aad582985 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 f6e0e7d8a007..de459411a0e8 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;
--
2.25.1