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 --- 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 +#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 back
RE: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
[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 Sent: Friday, March 25, 2022 2:58 PM To: Lin, Tsung-hua (Ryan) ; Wentland, Harry ; Li, Sun peng (Leo) ; Deucher, Alexander ; david1.z...@amd.com; airl...@linux.ie; dan...@ffwll.ch; seanp...@chromium.org; b...@basnieuwenhuizen.nl; Kazlauskas, Nicholas ; sas...@kernel.org; markyac...@google.com; victorchengchi...@amd.com; ching-shih...@amd.corp-partner.google.com; Siqueira, Rodrigo ; ddavenp...@chromium.org; amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Li, Leon 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 > > --- > 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 > +#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)
Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
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 Sent: Friday, March 25, 2022 2:58 PM To: Lin, Tsung-hua (Ryan) ; Wentland, Harry ; Li, Sun peng (Leo) ; Deucher, Alexander ; david1.z...@amd.com; airl...@linux.ie; dan...@ffwll.ch; seanp...@chromium.org; b...@basnieuwenhuizen.nl; Kazlauskas, Nicholas ; sas...@kernel.org; markyac...@google.com; victorchengchi...@amd.com; ching-shih...@amd.corp-partner.google.com; Siqueira, Rodrigo ; ddavenp...@chromium.org; amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Li, Leon 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 --- 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 +#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
Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
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 > > Sent: Friday, March 25, 2022 2:58 PM > > To: Lin, Tsung-hua (Ryan) ; Wentland, Harry > > ; Li, Sun peng (Leo) ; Deucher, > > Alexander ; david1.z...@amd.com; > > airl...@linux.ie; dan...@ffwll.ch; seanp...@chromium.org; > > b...@basnieuwenhuizen.nl; Kazlauskas, Nicholas > > ; sas...@kernel.org; markyac...@google.com; > > victorchengchi...@amd.com; ching-shih...@amd.corp-partner.google.com; > > Siqueira, Rodrigo ; ddavenp...@chromium.org; > > amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; > > linux-ker...@vger.kernel.org; Li, Leon > > 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 > > > > > > --- > > >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) { > > >
Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
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 Sent: Friday, March 25, 2022 2:58 PM To: Lin, Tsung-hua (Ryan) ; Wentland, Harry ; Li, Sun peng (Leo) ; Deucher, Alexander ; david1.z...@amd.com; airl...@linux.ie; dan...@ffwll.ch; seanp...@chromium.org; b...@basnieuwenhuizen.nl; Kazlauskas, Nicholas ; sas...@kernel.org; markyac...@google.com; victorchengchi...@amd.com; ching-shih...@amd.corp-partner.google.com; Siqueira, Rodrigo ; ddavenp...@chromium.org; amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Li, Leon 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 --- 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_coun
Re: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
On Fri, Mar 25, 2022 at 2:27 AM 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 > > --- > 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 > +#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_TARG
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 > > --- > 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 > +#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 b
RE: [PATCH v2 3/25] drm/amdgpu: Disable ABM when AC mode
[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 Sent: Friday, March 25, 2022 10:46 PM To: Lin, Tsung-hua (Ryan) ; Li, Sun peng (Leo) ; Deucher, Alexander ; Koenig, Christian ; david1.z...@amd.com; airl...@linux.ie; dan...@ffwll.ch; seanp...@chromium.org; b...@basnieuwenhuizen.nl; Kazlauskas, Nicholas ; sas...@kernel.org; markyac...@google.com; victorchengchi...@amd.com; ching-shih...@amd.corp-partner.google.com; Siqueira, Rodrigo ; ddavenp...@chromium.org; amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Li, Leon 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 > > --- > 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 > +#include "amdgpu.h" > #include "dmub_abm.h" > #include "dce_abm.h" > #in