[PATCH v2] drm/amd/powerplay: refine code in amd_powerplay.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
> Of Rex Zhu
> Sent: Thursday, September 28, 2017 1:06 AM
> To: amd-gfx at lists.freedesktop.org
> Cc: Zhu, Rex
> Subject: [PATCH v2] drm/amd/powerplay: refine code in amd_powerplay.c
> 
> v2: not set global module option amdgpu_dpm in
>     cgs interface and not set dpm_enabled in
>     function amdgpu_pp_hw_init.
> 
> delete flag of PP_DPM_DISABLED
> pp_en in pp_handle is enough
> 
> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>

Split this into two patches, one to remove PP_DPM_DISABLED and one to clean up the ret checks,  e.g.,
> -	if (ret != 0) {
> +	if (ret) {
With that fixed:
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c     |  12 --
>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c     | 153 ++++++++++---
> ---------
>  drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h |   2 -
>  3 files changed, 70 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> index 1649b1e..3b42f40 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> @@ -98,10 +98,6 @@ static int amdgpu_pp_early_init(void *handle)
>  					amd_pp->cgs_device ? amd_pp-
> >cgs_device :
>  					amd_pp->pp_handle);
> 
> -	if (ret == PP_DPM_DISABLED) {
> -		adev->pm.dpm_enabled = false;
> -		return 0;
> -	}
>  	return ret;
>  }
> 
> @@ -154,14 +150,6 @@ static int amdgpu_pp_hw_init(void *handle)
>  		ret = adev->powerplay.ip_funcs->hw_init(
>  					adev->powerplay.pp_handle);
> 
> -	if (ret == PP_DPM_DISABLED) {
> -		adev->pm.dpm_enabled = false;
> -		return 0;
> -	}
> -
> -	if ((amdgpu_dpm != 0) && !amdgpu_sriov_vf(adev))
> -		adev->pm.dpm_enabled = true;
> -
>  	return ret;
>  }
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index 488347a..8c725d2 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -41,11 +41,8 @@ static inline int pp_check(struct pp_instance *handle)
>  	if (handle->hwmgr == NULL || handle->hwmgr->smumgr_funcs ==
> NULL)
>  		return -EINVAL;
> 
> -	if (handle->pm_en == 0)
> -		return PP_DPM_DISABLED;
> -
>  	if (handle->hwmgr->hwmgr_func == NULL)
> -		return PP_DPM_DISABLED;
> +		handle->pm_en = 0;
> 
>  	return 0;
>  }
> @@ -96,14 +93,8 @@ static int pp_early_init(void *handle)
>  		return -EINVAL;
> 
>  	ret = hwmgr_early_init(pp_handle);
> -	if (ret)
> -		return -EINVAL;
> 
> -	if ((pp_handle->pm_en == 0)
> -		|| cgs_is_virtualization_enabled(pp_handle->device))
> -		return PP_DPM_DISABLED;
> -
> -	return 0;
> +	return ret;
>  }
> 
>  static int pp_sw_init(void *handle)
> @@ -114,7 +105,7 @@ static int pp_sw_init(void *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret == 0 || ret == PP_DPM_DISABLED) {
> +	if (!ret) {
>  		hwmgr = pp_handle->hwmgr;
> 
>  		if (hwmgr->smumgr_funcs->smu_init == NULL)
> @@ -134,7 +125,7 @@ static int pp_sw_fini(void *handle)
>  	struct pp_instance *pp_handle = (struct pp_instance *)handle;
> 
>  	ret = pp_check(pp_handle);
> -	if (ret == 0 || ret == PP_DPM_DISABLED) {
> +	if (!ret) {
>  		hwmgr = pp_handle->hwmgr;
> 
>  		if (hwmgr->smumgr_funcs->smu_fini == NULL)
> @@ -153,7 +144,7 @@ static int pp_hw_init(void *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret == 0 || ret == PP_DPM_DISABLED) {
> +	if (!ret) {
>  		hwmgr = pp_handle->hwmgr;
> 
>  		if (hwmgr->smumgr_funcs->start_smu == NULL)
> @@ -164,17 +155,16 @@ static int pp_hw_init(void *handle)
>  			hwmgr->smumgr_funcs->smu_fini(pp_handle-
> >hwmgr);
>  			return -EINVAL;;
>  		}
> -		if (ret == PP_DPM_DISABLED)
> -			return PP_DPM_DISABLED;
>  	}
> 
> -	ret = hwmgr_hw_init(pp_handle);
> -	if (ret)
> -		goto err;
> +	if (pp_handle->pm_en) {
> +		ret = hwmgr_hw_init(pp_handle);
> +		if (ret) {
> +			pp_handle->pm_en = 0;
> +			cgs_notify_dpm_enabled(hwmgr->device, false);
> +		}
> +	}
>  	return 0;
> -err:
> -	pp_handle->pm_en = 0;
> -	return PP_DPM_DISABLED;
>  }
> 
>  static int pp_hw_fini(void *handle)
> @@ -183,7 +173,7 @@ static int pp_hw_fini(void *handle)
>  	int ret = 0;
> 
>  	ret = pp_check(pp_handle);
> -	if (ret == 0)
> +	if (!ret && pp_handle->pm_en)
>  		hwmgr_hw_fini(pp_handle);
> 
>  	return 0;
> @@ -195,7 +185,8 @@ static int pp_late_init(void *handle)
>  	int ret = 0;
> 
>  	ret = pp_check(pp_handle);
> -	if (ret == 0)
> +
> +	if (!ret && pp_handle->pm_en)
>  		pp_dpm_dispatch_tasks(pp_handle,
>  					AMD_PP_TASK_COMPLETE_INIT,
> NULL, NULL);
> 
> @@ -232,7 +223,7 @@ int amd_set_clockgating_by_smu(void *handle,
> uint32_t msg_id)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -253,8 +244,7 @@ static int pp_set_powergating_state(void *handle,
>  	int ret = 0;
> 
>  	ret = pp_check(pp_handle);
> -
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -276,24 +266,25 @@ static int pp_suspend(void *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret == PP_DPM_DISABLED)
> -		return 0;
> -	else if (ret != 0)
> +	if (ret)
>  		return ret;
> 
> -	return hwmgr_hw_suspend(pp_handle);
> +	if (pp_handle->pm_en)
> +		ret = hwmgr_hw_suspend(pp_handle);
> +
> +	return ret;
>  }
> 
>  static int pp_resume(void *handle)
>  {
>  	struct pp_hwmgr  *hwmgr;
> -	int ret, ret1;
> +	int ret;
>  	struct pp_instance *pp_handle = (struct pp_instance *)handle;
> 
> -	ret1 = pp_check(pp_handle);
> +	ret = pp_check(pp_handle);
> 
> -	if (ret1 != 0 && ret1 != PP_DPM_DISABLED)
> -		return ret1;
> +	if (ret)
> +		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> 
> @@ -306,11 +297,10 @@ static int pp_resume(void *handle)
>  		hwmgr->smumgr_funcs->smu_fini(pp_handle->hwmgr);
>  		return ret;
>  	}
> +	if (pp_handle->pm_en)
> +		ret = hwmgr_hw_resume(pp_handle);
> 
> -	if (ret1 == PP_DPM_DISABLED)
> -		return 0;
> -
> -	return hwmgr_hw_resume(pp_handle);
> +	return ret;
>  }
> 
>  const struct amd_ip_funcs pp_ip_funcs = {
> @@ -386,7 +376,7 @@ static int pp_dpm_force_performance_level(void
> *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -421,7 +411,7 @@ static enum amd_dpm_forced_level
> pp_dpm_get_performance_level(
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -440,7 +430,7 @@ static uint32_t pp_dpm_get_sclk(void *handle, bool
> low)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -464,7 +454,7 @@ static uint32_t pp_dpm_get_mclk(void *handle, bool
> low)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -487,7 +477,7 @@ static void pp_dpm_powergate_vce(void *handle,
> bool gate)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -509,7 +499,7 @@ static void pp_dpm_powergate_uvd(void *handle,
> bool gate)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -531,7 +521,7 @@ static int pp_dpm_dispatch_tasks(void *handle,
> enum amd_pp_task task_id,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	mutex_lock(&pp_handle->pp_lock);
> @@ -551,7 +541,7 @@ static enum amd_pm_state_type
> pp_dpm_get_current_power_state(void *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -593,7 +583,7 @@ static void pp_dpm_set_fan_control_mode(void
> *handle, uint32_t mode)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -616,7 +606,7 @@ static uint32_t pp_dpm_get_fan_control_mode(void
> *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -639,7 +629,7 @@ static int pp_dpm_set_fan_speed_percent(void
> *handle, uint32_t percent)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -662,7 +652,7 @@ static int pp_dpm_get_fan_speed_percent(void
> *handle, uint32_t *speed)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -686,7 +676,7 @@ static int pp_dpm_get_fan_speed_rpm(void *handle,
> uint32_t *rpm)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -708,7 +698,7 @@ static int pp_dpm_get_temperature(void *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -733,7 +723,7 @@ static int pp_dpm_get_pp_num_states(void *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -778,7 +768,7 @@ static int pp_dpm_get_pp_table(void *handle, char
> **table)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -801,7 +791,7 @@ static int pp_dpm_set_pp_table(void *handle, const
> char *buf, size_t size)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -825,13 +815,10 @@ static int pp_dpm_set_pp_table(void *handle,
> const char *buf, size_t size)
>  	if (ret)
>  		return ret;
> 
> -	if (hwmgr->hwmgr_func->avfs_control) {
> +	if (hwmgr->hwmgr_func->avfs_control)
>  		ret = hwmgr->hwmgr_func->avfs_control(hwmgr, false);
> -		if (ret)
> -			return ret;
> -	}
> 
> -	return 0;
> +	return ret;
>  }
> 
>  static int pp_dpm_force_clock_level(void *handle,
> @@ -843,7 +830,7 @@ static int pp_dpm_force_clock_level(void *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -867,7 +854,7 @@ static int pp_dpm_print_clock_levels(void *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -890,7 +877,7 @@ static int pp_dpm_get_sclk_od(void *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -913,7 +900,7 @@ static int pp_dpm_set_sclk_od(void *handle, uint32_t
> value)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -937,7 +924,7 @@ static int pp_dpm_get_mclk_od(void *handle)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -960,7 +947,7 @@ static int pp_dpm_set_mclk_od(void *handle,
> uint32_t value)
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -984,7 +971,7 @@ static int pp_dpm_read_sensor(void *handle, int idx,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -1010,7 +997,7 @@ static int pp_dpm_read_sensor(void *handle, int
> idx,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return NULL;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -1190,11 +1177,11 @@ int amd_powerplay_reset(void *handle)
>  	struct pp_instance *instance = (struct pp_instance *)handle;
>  	int ret;
> 
> -	if (cgs_is_virtualization_enabled(instance->hwmgr->device))
> -		return PP_DPM_DISABLED;
> +	if (!instance->pm_en)
> +		return 0;
> 
>  	ret = pp_check(instance);
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	ret = pp_hw_fini(instance);
> @@ -1203,7 +1190,7 @@ int amd_powerplay_reset(void *handle)
> 
>  	ret = hwmgr_hw_init(instance);
>  	if (ret)
> -		return PP_DPM_DISABLED;
> +		return ret;
> 
>  	return hwmgr_handle_task(instance,
> AMD_PP_TASK_COMPLETE_INIT, NULL, NULL);
>  }
> @@ -1219,7 +1206,7 @@ int
> amd_powerplay_display_configuration_change(void *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -1238,7 +1225,7 @@ int amd_powerplay_get_display_power_level(void
> *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -1263,7 +1250,7 @@ int amd_powerplay_get_current_clocks(void
> *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -1280,7 +1267,7 @@ int amd_powerplay_get_current_clocks(void
> *handle,
>  		ret = phm_get_clock_info(hwmgr, &hwmgr->current_ps-
> >hardware,
>  					&hw_clocks,
> PHM_PerformanceLevelDesignation_Activity);
> 
> -	if (ret != 0) {
> +	if (ret) {
>  		pr_info("Error in phm_get_clock_info \n");
>  		mutex_unlock(&pp_handle->pp_lock);
>  		return -EINVAL;
> @@ -1314,7 +1301,7 @@ int amd_powerplay_get_clock_by_type(void
> *handle, enum amd_pp_clock_type type, s
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> @@ -1337,7 +1324,7 @@ int
> amd_powerplay_get_clock_by_type_with_latency(void *handle,
>  	int ret = 0;
> 
>  	ret = pp_check(pp_handle);
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	if (!clocks)
> @@ -1359,7 +1346,7 @@ int
> amd_powerplay_get_clock_by_type_with_voltage(void *handle,
>  	int ret = 0;
> 
>  	ret = pp_check(pp_handle);
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	if (!clocks)
> @@ -1383,7 +1370,7 @@ int
> amd_powerplay_set_watermarks_for_clocks_ranges(void *handle,
>  	int ret = 0;
> 
>  	ret = pp_check(pp_handle);
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	if (!wm_with_clock_ranges)
> @@ -1407,7 +1394,7 @@ int
> amd_powerplay_display_clock_voltage_request(void *handle,
>  	int ret = 0;
> 
>  	ret = pp_check(pp_handle);
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	if (!clock)
> @@ -1431,7 +1418,7 @@ int
> amd_powerplay_get_display_mode_validation_clocks(void *handle,
> 
>  	ret = pp_check(pp_handle);
> 
> -	if (ret != 0)
> +	if (ret)
>  		return ret;
> 
>  	hwmgr = pp_handle->hwmgr;
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
> b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
> index 916b6c4..e52adc8 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
> @@ -33,8 +33,6 @@
>  extern const struct amd_ip_funcs pp_ip_funcs;
>  extern const struct amd_pm_funcs pp_dpm_funcs;
> 
> -#define PP_DPM_DISABLED 0xCCCC
> -
>  enum amd_pp_sensors {
>  	AMDGPU_PP_SENSOR_GFX_SCLK = 0,
>  	AMDGPU_PP_SENSOR_VDDNB,
> --
> 1.9.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux