[PATCH V5 15/16] drm/amd/pm: revise the performance level setting APIs

Quan, Evan Evan.Quan at amd.com
Mon Dec 13 05:45:24 UTC 2021


[Public]



> -----Original Message-----
> From: Chen, Guchun <Guchun.Chen at amd.com>
> Sent: Monday, December 13, 2021 1:15 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Lazar, Lijo
> <Lijo.Lazar at amd.com>; Quan, Evan <Evan.Quan at amd.com>
> Subject: RE: [PATCH V5 15/16] drm/amd/pm: revise the performance level
> setting APIs
> 
> [Public]
> 
> A coding style nitpick.
> 
> 	int ret = 0;
> +	uint32_t profile_mode_mask =
> AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD |
> +
> 	AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK |
> +
> 	AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK |
> +
> 	AMD_DPM_FORCED_LEVEL_PROFILE_PEAK;
> 
> It's better to declare short variable at the end. So pls move "int ret = 0;" after
> profile_mode_mask.
[Quan, Evan] Sure, will update that.

BR
Evan
> 
> Regards,
> Guchun
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Evan
> Quan
> Sent: Monday, December 13, 2021 11:52 AM
> To: amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Lazar, Lijo
> <Lijo.Lazar at amd.com>; Quan, Evan <Evan.Quan at amd.com>
> Subject: [PATCH V5 15/16] drm/amd/pm: revise the performance level
> setting APIs
> 
> Avoid cross callings which make lock protection enforcement on
> amdgpu_dpm_force_performance_level() impossible.
> 
> Signed-off-by: Evan Quan <evan.quan at amd.com>
> Change-Id: Ie658140f40ab906ce2ec47576a086062b61076a6
> --
> v1->v2:
>   - drop unused enable_umd_pstate callback(Lijo)
> ---
>  drivers/gpu/drm/amd/include/amd_shared.h      |  2 --
>  drivers/gpu/drm/amd/pm/amdgpu_pm.c            | 29 ++++++++++++++++---
>  .../gpu/drm/amd/pm/legacy-dpm/legacy_dpm.c    | 17 ++++++-----
>  .../gpu/drm/amd/pm/powerplay/amd_powerplay.c  | 12 --------
>  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 15 ----------
>  drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h |  1 -
>  6 files changed, 34 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h
> b/drivers/gpu/drm/amd/include/amd_shared.h
> index f57a1478f0fe..fb6ad56ad6f1 100644
> --- a/drivers/gpu/drm/amd/include/amd_shared.h
> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> @@ -268,7 +268,6 @@ enum amd_dpm_forced_level;
>   * @set_clockgating_state: enable/disable cg for the IP block
>   * @set_powergating_state: enable/disable pg for the IP block
>   * @get_clockgating_state: get current clockgating status
> - * @enable_umd_pstate: enable UMD powerstate
>   *
>   * These hooks provide an interface for controlling the operational state
>   * of IP blocks. After acquiring a list of IP blocks for the GPU in use, @@ -
> 299,7 +298,6 @@ struct amd_ip_funcs {
>  	int (*set_powergating_state)(void *handle,
>  				     enum amd_powergating_state state);
>  	void (*get_clockgating_state)(void *handle, u32 *flags);
> -	int (*enable_umd_pstate)(void *handle, enum
> amd_dpm_forced_level *level);
>  };
> 
> 
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index ce80430c0eb6..106f6ee955f4 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -301,6 +301,10 @@ static ssize_t
> amdgpu_set_power_dpm_force_performance_level(struct device *dev,
>  	enum amd_dpm_forced_level level;
>  	enum amd_dpm_forced_level current_level;
>  	int ret = 0;
> +	uint32_t profile_mode_mask =
> AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD |
> +
> 	AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK |
> +
> 	AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK |
> +
> 	AMD_DPM_FORCED_LEVEL_PROFILE_PEAK;
> 
>  	if (amdgpu_in_reset(adev))
>  		return -EPERM;
> @@ -354,10 +358,7 @@ static ssize_t
> amdgpu_set_power_dpm_force_performance_level(struct device *dev,
>  	}
> 
>  	/* profile_exit setting is valid only when current mode is in profile
> mode */
> -	if (!(current_level &
> (AMD_DPM_FORCED_LEVEL_PROFILE_STANDARD |
> -	    AMD_DPM_FORCED_LEVEL_PROFILE_MIN_SCLK |
> -	    AMD_DPM_FORCED_LEVEL_PROFILE_MIN_MCLK |
> -	    AMD_DPM_FORCED_LEVEL_PROFILE_PEAK)) &&
> +	if (!(current_level & profile_mode_mask) &&
>  	    (level == AMD_DPM_FORCED_LEVEL_PROFILE_EXIT)) {
>  		pr_err("Currently not in any profile mode!\n");
>  		pm_runtime_mark_last_busy(ddev->dev);
> @@ -365,6 +366,26 @@ static ssize_t
> amdgpu_set_power_dpm_force_performance_level(struct device *dev,
>  		return -EINVAL;
>  	}
> 
> +	if (!(current_level & profile_mode_mask) &&
> +	      (level & profile_mode_mask)) {
> +		/* enter UMD Pstate */
> +		amdgpu_device_ip_set_powergating_state(adev,
> +
> AMD_IP_BLOCK_TYPE_GFX,
> +
> AMD_PG_STATE_UNGATE);
> +		amdgpu_device_ip_set_clockgating_state(adev,
> +
> AMD_IP_BLOCK_TYPE_GFX,
> +
> AMD_CG_STATE_UNGATE);
> +	} else if ((current_level & profile_mode_mask) &&
> +		    !(level & profile_mode_mask)) {
> +		/* exit UMD Pstate */
> +		amdgpu_device_ip_set_clockgating_state(adev,
> +
> AMD_IP_BLOCK_TYPE_GFX,
> +						       AMD_CG_STATE_GATE);
> +		amdgpu_device_ip_set_powergating_state(adev,
> +
> AMD_IP_BLOCK_TYPE_GFX,
> +						       AMD_PG_STATE_GATE);
> +	}
> +
>  	if (amdgpu_dpm_force_performance_level(adev, level)) {
>  		pm_runtime_mark_last_busy(ddev->dev);
>  		pm_runtime_put_autosuspend(ddev->dev);
> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/legacy_dpm.c
> b/drivers/gpu/drm/amd/pm/legacy-dpm/legacy_dpm.c
> index 3c6ee493e410..9613c6181c17 100644
> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/legacy_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/legacy_dpm.c
> @@ -953,6 +953,7 @@ static struct amdgpu_ps
> *amdgpu_dpm_pick_power_state(struct amdgpu_device *adev,
> 
>  static int amdgpu_dpm_change_power_state_locked(struct
> amdgpu_device *adev)  {
> +	const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
>  	struct amdgpu_ps *ps;
>  	enum amd_pm_state_type dpm_state;
>  	int ret;
> @@ -976,7 +977,7 @@ static int
> amdgpu_dpm_change_power_state_locked(struct amdgpu_device *adev)
>  	else
>  		return -EINVAL;
> 
> -	if (amdgpu_dpm == 1 && adev->powerplay.pp_funcs-
> >print_power_state) {
> +	if (amdgpu_dpm == 1 && pp_funcs->print_power_state) {
>  		printk("switching from power state:\n");
>  		amdgpu_dpm_print_power_state(adev, adev-
> >pm.dpm.current_ps);
>  		printk("switching to power state:\n"); @@ -985,14 +986,14
> @@ static int amdgpu_dpm_change_power_state_locked(struct
> amdgpu_device *adev)
> 
>  	/* update whether vce is active */
>  	ps->vce_active = adev->pm.dpm.vce_active;
> -	if (adev->powerplay.pp_funcs->display_configuration_changed)
> +	if (pp_funcs->display_configuration_changed)
>  		amdgpu_dpm_display_configuration_changed(adev);
> 
>  	ret = amdgpu_dpm_pre_set_power_state(adev);
>  	if (ret)
>  		return ret;
> 
> -	if (adev->powerplay.pp_funcs->check_state_equal) {
> +	if (pp_funcs->check_state_equal) {
>  		if (0 != amdgpu_dpm_check_state_equal(adev, adev-
> >pm.dpm.current_ps, adev->pm.dpm.requested_ps, &equal))
>  			equal = false;
>  	}
> @@ -1000,24 +1001,24 @@ static int
> amdgpu_dpm_change_power_state_locked(struct amdgpu_device *adev)
>  	if (equal)
>  		return 0;
> 
> -	if (adev->powerplay.pp_funcs->set_power_state)
> -		adev->powerplay.pp_funcs->set_power_state(adev-
> >powerplay.pp_handle);
> +	if (pp_funcs->set_power_state)
> +		pp_funcs->set_power_state(adev->powerplay.pp_handle);
> 
>  	amdgpu_dpm_post_set_power_state(adev);
> 
>  	adev->pm.dpm.current_active_crtcs = adev-
> >pm.dpm.new_active_crtcs;
>  	adev->pm.dpm.current_active_crtc_count = adev-
> >pm.dpm.new_active_crtc_count;
> 
> -	if (adev->powerplay.pp_funcs->force_performance_level) {
> +	if (pp_funcs->force_performance_level) {
>  		if (adev->pm.dpm.thermal_active) {
>  			enum amd_dpm_forced_level level = adev-
> >pm.dpm.forced_level;
>  			/* force low perf level for thermal */
> -			amdgpu_dpm_force_performance_level(adev,
> AMD_DPM_FORCED_LEVEL_LOW);
> +			pp_funcs->force_performance_level(adev,
> AMD_DPM_FORCED_LEVEL_LOW);
>  			/* save the user's level */
>  			adev->pm.dpm.forced_level = level;
>  		} else {
>  			/* otherwise, user selected level */
> -			amdgpu_dpm_force_performance_level(adev,
> adev->pm.dpm.forced_level);
> +			pp_funcs->force_performance_level(adev, adev-
> >pm.dpm.forced_level);
>  		}
>  	}
> 
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> index d57d5c28c013..5a14ddd3ef05 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> @@ -323,12 +323,6 @@ static void pp_dpm_en_umd_pstate(struct
> pp_hwmgr  *hwmgr,
>  		if (*level & profile_mode_mask) {
>  			hwmgr->saved_dpm_level = hwmgr->dpm_level;
>  			hwmgr->en_umd_pstate = true;
> -			amdgpu_device_ip_set_powergating_state(hwmgr-
> >adev,
> -					AMD_IP_BLOCK_TYPE_GFX,
> -					AMD_PG_STATE_UNGATE);
> -			amdgpu_device_ip_set_clockgating_state(hwmgr-
> >adev,
> -						AMD_IP_BLOCK_TYPE_GFX,
> -						AMD_CG_STATE_UNGATE);
>  		}
>  	} else {
>  		/* exit umd pstate, restore level, enable gfx cg*/ @@ -336,12
> +330,6 @@ static void pp_dpm_en_umd_pstate(struct pp_hwmgr  *hwmgr,
>  			if (*level ==
> AMD_DPM_FORCED_LEVEL_PROFILE_EXIT)
>  				*level = hwmgr->saved_dpm_level;
>  			hwmgr->en_umd_pstate = false;
> -			amdgpu_device_ip_set_clockgating_state(hwmgr-
> >adev,
> -					AMD_IP_BLOCK_TYPE_GFX,
> -					AMD_CG_STATE_GATE);
> -			amdgpu_device_ip_set_powergating_state(hwmgr-
> >adev,
> -					AMD_IP_BLOCK_TYPE_GFX,
> -					AMD_PG_STATE_GATE);
>  		}
>  	}
>  }
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index b5fbad92738b..29f521854796 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -1674,14 +1674,7 @@ static int smu_enable_umd_pstate(void *handle,
>  		/* enter umd pstate, save current level, disable gfx cg*/
>  		if (*level & profile_mode_mask) {
>  			smu_dpm_ctx->saved_dpm_level = smu_dpm_ctx-
> >dpm_level;
> -			smu_dpm_ctx->enable_umd_pstate = true;
>  			smu_gpo_control(smu, false);
> -			amdgpu_device_ip_set_powergating_state(smu-
> >adev,
> -
> AMD_IP_BLOCK_TYPE_GFX,
> -
> AMD_PG_STATE_UNGATE);
> -			amdgpu_device_ip_set_clockgating_state(smu-
> >adev,
> -
> AMD_IP_BLOCK_TYPE_GFX,
> -
> AMD_CG_STATE_UNGATE);
>  			smu_gfx_ulv_control(smu, false);
>  			smu_deep_sleep_control(smu, false);
>  			amdgpu_asic_update_umd_stable_pstate(smu-
> >adev, true); @@ -1691,16 +1684,9 @@ static int
> smu_enable_umd_pstate(void *handle,
>  		if (!(*level & profile_mode_mask)) {
>  			if (*level ==
> AMD_DPM_FORCED_LEVEL_PROFILE_EXIT)
>  				*level = smu_dpm_ctx->saved_dpm_level;
> -			smu_dpm_ctx->enable_umd_pstate = false;
>  			amdgpu_asic_update_umd_stable_pstate(smu-
> >adev, false);
>  			smu_deep_sleep_control(smu, true);
>  			smu_gfx_ulv_control(smu, true);
> -			amdgpu_device_ip_set_clockgating_state(smu-
> >adev,
> -
> AMD_IP_BLOCK_TYPE_GFX,
> -
> AMD_CG_STATE_GATE);
> -			amdgpu_device_ip_set_powergating_state(smu-
> >adev,
> -
> AMD_IP_BLOCK_TYPE_GFX,
> -
> AMD_PG_STATE_GATE);
>  			smu_gpo_control(smu, true);
>  		}
>  	}
> @@ -2146,7 +2132,6 @@ const struct amd_ip_funcs smu_ip_funcs = {
>  	.soft_reset = NULL,
>  	.set_clockgating_state = smu_set_clockgating_state,
>  	.set_powergating_state = smu_set_powergating_state,
> -	.enable_umd_pstate = smu_enable_umd_pstate,
>  };
> 
>  const struct amdgpu_ip_block_version smu_v11_0_ip_block = diff --git
> a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> index 9d4a85c39ad2..778196167de2 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> @@ -363,7 +363,6 @@ struct smu_dpm_context {
>  	uint32_t dpm_context_size;
>  	void *dpm_context;
>  	void *golden_dpm_context;
> -	bool enable_umd_pstate;
>  	enum amd_dpm_forced_level dpm_level;
>  	enum amd_dpm_forced_level saved_dpm_level;
>  	enum amd_dpm_forced_level requested_dpm_level;
> --
> 2.29.0


More information about the amd-gfx mailing list