[PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Ryan Lin]

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 <tsung-hua.lin@amd.com>

---
 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

Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Christian König]

Am 25.03.22 um 05:05 schrieb Ryan Lin:
> 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 <tsung-hua.lin@amd.com>
>
> ---
>   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) {

String comparison in a hot path is not something we usually like to see 
in the kernel.

Isn't there any other way to detect that? Like a flag or similar?

Regards,
Christian.

> +		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;
>   

RE: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Lin, Tsung-hua (Ryan)]

[AMD Official Use Only]

Hi Christian,

There is already a string comparison in the same function. I just reference that to port this solution.



#define ACPI_AC_CLASS           "ac_adapter"


static int amdgpu_acpi_event(struct notifier_block *nb,
			     unsigned long val,
			     void *data)
{
	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) {      <-------------------here!
		if (power_supply_is_system_supplied() > 0)
			DRM_DEBUG_DRIVER("pm: AC\n");
		else
			DRM_DEBUG_DRIVER("pm: DC\n");

		amdgpu_pm_acpi_event_handler(adev);
	}

	/* Check for pending SBIOS requests */
	return amdgpu_atif_handler(adev, entry);
}

Thanks,
Ryan Lin.

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com> 
Sent: Friday, March 25, 2022 2:58 PM
To: Lin, Tsung-hua (Ryan) <Tsung-hua.Lin@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; David1.Zhou@amd.com; airlied@linux.ie; daniel@ffwll.ch; seanpaul@chromium.org; bas@basnieuwenhuizen.nl; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; sashal@kernel.org; markyacoub@google.com; victorchengchi.lu@amd.com; ching-shih.li@amd.corp-partner.google.com; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; ddavenport@chromium.org; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Li, Leon <Leon.Li@amd.com>
Subject: Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

Am 25.03.22 um 05:05 schrieb Ryan Lin:
> 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 <tsung-hua.lin@amd.com>
>
> ---
>   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) {

String comparison in a hot path is not something we usually like to see in the kernel.

Isn't there any other way to detect that? Like a flag or similar?

Regards,
Christian.

> +		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;
>   

Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Christian König]

Hi Ryan,

we should try to avoid that and if it isn't possible at least use some 
constant like ACPI_AC_CLASS.

Could be that the information isn't available otherwise. Alex should 
know more about that.

Regards,
Christian.

Am 25.03.22 um 08:09 schrieb Lin, Tsung-hua (Ryan):
> [AMD Official Use Only]
>
> Hi Christian,
>
> There is already a string comparison in the same function. I just reference that to port this solution.
>
>
>
> #define ACPI_AC_CLASS           "ac_adapter"
>
>
> static int amdgpu_acpi_event(struct notifier_block *nb,
> 			     unsigned long val,
> 			     void *data)
> {
> 	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) {      <-------------------here!
> 		if (power_supply_is_system_supplied() > 0)
> 			DRM_DEBUG_DRIVER("pm: AC\n");
> 		else
> 			DRM_DEBUG_DRIVER("pm: DC\n");
>
> 		amdgpu_pm_acpi_event_handler(adev);
> 	}
>
> 	/* Check for pending SBIOS requests */
> 	return amdgpu_atif_handler(adev, entry);
> }
>
> Thanks,
> Ryan Lin.
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, March 25, 2022 2:58 PM
> To: Lin, Tsung-hua (Ryan) <Tsung-hua.Lin@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; David1.Zhou@amd.com; airlied@linux.ie; daniel@ffwll.ch; seanpaul@chromium.org; bas@basnieuwenhuizen.nl; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; sashal@kernel.org; markyacoub@google.com; victorchengchi.lu@amd.com; ching-shih.li@amd.corp-partner.google.com; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; ddavenport@chromium.org; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Li, Leon <Leon.Li@amd.com>
> Subject: Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
>
> Am 25.03.22 um 05:05 schrieb Ryan Lin:
>> 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 <tsung-hua.lin@amd.com>
>>
>> ---
>>    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) {
> String comparison in a hot path is not something we usually like to see in the kernel.
>
> Isn't there any other way to detect that? Like a flag or similar?
>
> Regards,
> Christian.
>
>> +		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;
>>    

Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Daniel Vetter]

On Fri, Mar 25, 2022 at 08:22:29AM +0100, Christian König wrote:
> Hi Ryan,
> 
> we should try to avoid that and if it isn't possible at least use some
> constant like ACPI_AC_CLASS.
> 
> Could be that the information isn't available otherwise. Alex should know
> more about that.

I wonder whether we shouldn't need a more dedicated notification from acpi
for power supply events instead of stitching this together ourselves. At
least this kind of stuff feels more into the policy/tuning territory where
a bit more careful interfaces might be good instead of just "hey there's
this very funny acpi protocol we just have to take part in to not upset
the hw/fw".
-Daniel

> 
> Regards,
> Christian.
> 
> Am 25.03.22 um 08:09 schrieb Lin, Tsung-hua (Ryan):
> > [AMD Official Use Only]
> > 
> > Hi Christian,
> > 
> > There is already a string comparison in the same function. I just reference that to port this solution.
> > 
> > 
> > 
> > #define ACPI_AC_CLASS           "ac_adapter"
> > 
> > 
> > static int amdgpu_acpi_event(struct notifier_block *nb,
> > 			     unsigned long val,
> > 			     void *data)
> > {
> > 	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) {      <-------------------here!
> > 		if (power_supply_is_system_supplied() > 0)
> > 			DRM_DEBUG_DRIVER("pm: AC\n");
> > 		else
> > 			DRM_DEBUG_DRIVER("pm: DC\n");
> > 
> > 		amdgpu_pm_acpi_event_handler(adev);
> > 	}
> > 
> > 	/* Check for pending SBIOS requests */
> > 	return amdgpu_atif_handler(adev, entry);
> > }
> > 
> > Thanks,
> > Ryan Lin.
> > 
> > -----Original Message-----
> > From: Koenig, Christian <Christian.Koenig@amd.com>
> > Sent: Friday, March 25, 2022 2:58 PM
> > To: Lin, Tsung-hua (Ryan) <Tsung-hua.Lin@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; David1.Zhou@amd.com; airlied@linux.ie; daniel@ffwll.ch; seanpaul@chromium.org; bas@basnieuwenhuizen.nl; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; sashal@kernel.org; markyacoub@google.com; victorchengchi.lu@amd.com; ching-shih.li@amd.corp-partner.google.com; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; ddavenport@chromium.org; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Li, Leon <Leon.Li@amd.com>
> > Subject: Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
> > 
> > Am 25.03.22 um 05:05 schrieb Ryan Lin:
> > > 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 <tsung-hua.lin@amd.com>
> > > 
> > > ---
> > >    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) {
> > String comparison in a hot path is not something we usually like to see in the kernel.
> > 
> > Isn't there any other way to detect that? Like a flag or similar?
> > 
> > Regards,
> > Christian.
> > 
> > > +		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;
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Christian König]

Am 25.03.22 um 10:49 schrieb Daniel Vetter:
> On Fri, Mar 25, 2022 at 08:22:29AM +0100, Christian König wrote:
>> Hi Ryan,
>>
>> we should try to avoid that and if it isn't possible at least use some
>> constant like ACPI_AC_CLASS.
>>
>> Could be that the information isn't available otherwise. Alex should know
>> more about that.
> I wonder whether we shouldn't need a more dedicated notification from acpi
> for power supply events instead of stitching this together ourselves. At
> least this kind of stuff feels more into the policy/tuning territory where
> a bit more careful interfaces might be good instead of just "hey there's
> this very funny acpi protocol we just have to take part in to not upset
> the hw/fw".

That is pretty much my thinking as well.

A quick grep shows that both amdgpu, radeon, nouveau and i915 all parse 
the same information with self defined macros and strcmp(). That's not 
really the way we usually do stuff like this.

Ideally the ACPI layer in the core kernel would parse the information 
and give it as enum or flags to the drivers instead.

At bare minimum we should move all the ACPI_AC_CLASS, ACPI_VIDEO_CLASS 
and raw strings into a common place to start with.

Regards,
Christian.

> -Daniel
>
>> Regards,
>> Christian.
>>
>> Am 25.03.22 um 08:09 schrieb Lin, Tsung-hua (Ryan):
>>> [AMD Official Use Only]
>>>
>>> Hi Christian,
>>>
>>> There is already a string comparison in the same function. I just reference that to port this solution.
>>>
>>>
>>>
>>> #define ACPI_AC_CLASS           "ac_adapter"
>>>
>>>
>>> static int amdgpu_acpi_event(struct notifier_block *nb,
>>> 			     unsigned long val,
>>> 			     void *data)
>>> {
>>> 	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) {      <-------------------here!
>>> 		if (power_supply_is_system_supplied() > 0)
>>> 			DRM_DEBUG_DRIVER("pm: AC\n");
>>> 		else
>>> 			DRM_DEBUG_DRIVER("pm: DC\n");
>>>
>>> 		amdgpu_pm_acpi_event_handler(adev);
>>> 	}
>>>
>>> 	/* Check for pending SBIOS requests */
>>> 	return amdgpu_atif_handler(adev, entry);
>>> }
>>>
>>> Thanks,
>>> Ryan Lin.
>>>
>>> -----Original Message-----
>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>> Sent: Friday, March 25, 2022 2:58 PM
>>> To: Lin, Tsung-hua (Ryan) <Tsung-hua.Lin@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; David1.Zhou@amd.com; airlied@linux.ie; daniel@ffwll.ch; seanpaul@chromium.org; bas@basnieuwenhuizen.nl; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; sashal@kernel.org; markyacoub@google.com; victorchengchi.lu@amd.com; ching-shih.li@amd.corp-partner.google.com; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; ddavenport@chromium.org; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Li, Leon <Leon.Li@amd.com>
>>> Subject: Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
>>>
>>> Am 25.03.22 um 05:05 schrieb Ryan Lin:
>>>> 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 <tsung-hua.lin@amd.com>
>>>>
>>>> ---
>>>>     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) {
>>> String comparison in a hot path is not something we usually like to see in the kernel.
>>>
>>> Isn't there any other way to detect that? Like a flag or similar?
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +		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;

Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Alex Deucher]

On Fri, Mar 25, 2022 at 2:27 AM Ryan Lin <tsung-hua.lin@amd.com> 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 <tsung-hua.lin@amd.com>
>
> ---
>  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;
> +       }
> +

We already set adev->pm.ac_power in amdgpu_pm_acpi_event_handler()
which gets called a few lines below.

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
>

Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Harry Wentland]

On 2022-03-25 00:05, Ryan Lin wrote:
> Disable ABM feature when the system is running on AC mode to get
> the more perfect contrast of the display.

It says patch 3 out of 25. Are there other patches? If so, I can't
find them in my mailbox and neither can patchwork
https://patchwork.freedesktop.org/series/101767/
 
> Signed-off-by: Ryan Lin <tsung-hua.lin@amd.com>
> 
> ---
>  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 file lives in DC, which is shared code between Windows and Linux. We
cannot directly use adev here. Any information needs to go through DC structs.

I seem to remember someone saying that ABM gets disabled on Windows when
we're in AC mode. Have you checked with our Windows guys about this? I feel
we're re-inventing the wheel here for no good reason.

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;
>  

RE: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode

[Lin, Tsung-hua (Ryan)]

[AMD Official Use Only]

Hi Harry,

I have been reminded to do some modifications to the patch format. So the 3/25 it's the date I resent the mail v2.
If the mail title usage is not correct, please let me know.

And about the questions:
" This file lives in DC, which is shared code between Windows and Linux. We cannot directly use adev here. Any information needs to go through DC structs."
	- I use the adev here because I just reference the code I found in the same function. 

 +	if (strcmp(entry->device_class, "battery") == 0) {                             <-------- I added.
 +		adev->pm.ac_power = power_supply_is_system_supplied() > 0;
 +	}
 +
  	if (strcmp(entry->device_class, ACPI_AC_CLASS) == 0) {                  <-------- I found.
  		if (power_supply_is_system_supplied() > 0)
  			DRM_DEBUG_DRIVER("pm: AC\n");

	-And the reason why I need to add another comparison is that the string of the device_class I got is always "battery" when I plug/unplug the ac cable.
	 It never reports "ac_adapter" to me. So I add the "battery" line to do that.


"I seem to remember someone saying that ABM gets disabled on Windows when we're in AC mode. Have you checked with our Windows guys about this? I feel we're re-inventing the wheel here for no good reason."

	-Yes, I have asked the Windows guys and they told me the ABM should be disabled in AC mode. But seems we don’t have this function on Linux, so I implemented this to disable ABM in AC mode.

Thanks,
Ryan Lin.


-----Original Message-----
From: Wentland, Harry <Harry.Wentland@amd.com> 
Sent: Friday, March 25, 2022 10:46 PM
To: Lin, Tsung-hua (Ryan) <Tsung-hua.Lin@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; David1.Zhou@amd.com; airlied@linux.ie; daniel@ffwll.ch; seanpaul@chromium.org; bas@basnieuwenhuizen.nl; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; sashal@kernel.org; markyacoub@google.com; victorchengchi.lu@amd.com; ching-shih.li@amd.corp-partner.google.com; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; ddavenport@chromium.org; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Li, Leon <Leon.Li@amd.com>
Subject: Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode



On 2022-03-25 00:05, Ryan Lin wrote:
> Disable ABM feature when the system is running on AC mode to get the 
> more perfect contrast of the display.

It says patch 3 out of 25. Are there other patches? If so, I can't find them in my mailbox and neither can patchwork https://patchwork.freedesktop.org/series/101767/
 
> Signed-off-by: Ryan Lin <tsung-hua.lin@amd.com>
> 
> ---
>  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 file lives in DC, which is shared code between Windows and Linux. We cannot directly use adev here. Any information needs to go through DC structs.

I seem to remember someone saying that ABM gets disabled on Windows when we're in AC mode. Have you checked with our Windows guys about this? I feel we're re-inventing the wheel here for no good reason.

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;
>