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

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

2022-03-24 Thread 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 

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

2022-03-25 Thread 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)

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

2022-03-25 Thread 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 
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

2022-03-25 Thread 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 
> > 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

2022-03-25 Thread 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 
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

2022-03-25 Thread Alex Deucher

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

2022-03-25 Thread 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 
> 
> ---
>  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

2022-03-27 Thread 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  
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

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

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

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

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

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

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

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

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

8 matches

Site Navigation

Mail list logo

Footer information