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

[Christian König <christian.koenig@xxxxxxx>]

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@xxxxxxx>
> > > Sent: Friday, March 25, 2022 2:58 PM
> > > To: Lin, Tsung-hua (Ryan) <Tsung-hua.Lin@xxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>; Li, Sun peng (Leo) <Sunpeng.Li@xxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; David1.Zhou@xxxxxxx; airlied@xxxxxxxx; daniel@xxxxxxxx; seanpaul@xxxxxxxxxxxx; bas@xxxxxxxxxxxxxxxxxxx; Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>; sashal@xxxxxxxxxx; markyacoub@xxxxxxxxxx; victorchengchi.lu@xxxxxxx; ching-shih.li@xxxxxxxxxxxxxxxxxxxxxxxxxxx; Siqueira, Rodrigo <Rodrigo.Siqueira@xxxxxxx>; ddavenport@xxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Li, Leon <Leon.Li@xxxxxxx>
> > > 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@xxxxxxx>
> > > >
> > > > ---
> > > >     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;