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