[PATCH v2 1/3] drm/amdgpu/kv,si: DPM: Use new print helpers

[PATCH v2 1/3] drm/amdgpu/kv,si: DPM: Use new print helpers

[Paul Menzel]

---
Untested.

 drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 58 ++++++++++----------
 drivers/gpu/drm/amd/amdgpu/si_dpm.c | 82 ++++++++++++++---------------
 2 files changed, 70 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
index 4b3faaccecb9..b179bdc17cdc 100644
--- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
@@ -1252,7 +1252,7 @@ static void kv_dpm_enable_bapm(void *handle, bool enable)
 	if (pi->bapm_enable) {
 		ret = amdgpu_kv_smc_bapm_enable(adev, enable);
 		if (ret)
-			DRM_ERROR("amdgpu_kv_smc_bapm_enable failed\n");
+			drm_err(adev, "amdgpu_kv_smc_bapm_enable failed\n");
 	}
 }
 
@@ -1263,40 +1263,40 @@ static int kv_dpm_enable(struct amdgpu_device *adev)
 
 	ret = kv_process_firmware_header(adev);
 	if (ret) {
-		DRM_ERROR("kv_process_firmware_header failed\n");
+		drm_err(adev, "kv_process_firmware_header failed\n");
 		return ret;
 	}
 	kv_init_fps_limits(adev);
 	kv_init_graphics_levels(adev);
 	ret = kv_program_bootup_state(adev);
 	if (ret) {
-		DRM_ERROR("kv_program_bootup_state failed\n");
+		dev_err(adev, "kv_program_bootup_state failed\n");
 		return ret;
 	}
 	kv_calculate_dfs_bypass_settings(adev);
 	ret = kv_upload_dpm_settings(adev);
 	if (ret) {
-		DRM_ERROR("kv_upload_dpm_settings failed\n");
+		dev_err(adev, "kv_upload_dpm_settings failed\n");
 		return ret;
 	}
 	ret = kv_populate_uvd_table(adev);
 	if (ret) {
-		DRM_ERROR("kv_populate_uvd_table failed\n");
+		drm_err(adev, "kv_populate_uvd_table failed\n");
 		return ret;
 	}
 	ret = kv_populate_vce_table(adev);
 	if (ret) {
-		DRM_ERROR("kv_populate_vce_table failed\n");
+		drm_err(adev, "kv_populate_vce_table failed\n");
 		return ret;
 	}
 	ret = kv_populate_samu_table(adev);
 	if (ret) {
-		DRM_ERROR("kv_populate_samu_table failed\n");
+		drm_err(adev, "kv_populate_samu_table failed\n");
 		return ret;
 	}
 	ret = kv_populate_acp_table(adev);
 	if (ret) {
-		DRM_ERROR("kv_populate_acp_table failed\n");
+		drm_err(adev, "kv_populate_acp_table failed\n");
 		return ret;
 	}
 	kv_program_vc(adev);
@@ -1307,39 +1307,39 @@ static int kv_dpm_enable(struct amdgpu_device *adev)
 	if (pi->enable_auto_thermal_throttling) {
 		ret = kv_enable_auto_thermal_throttling(adev);
 		if (ret) {
-			DRM_ERROR("kv_enable_auto_thermal_throttling failed\n");
+			drm_err(adev, "kv_enable_auto_thermal_throttling failed\n");
 			return ret;
 		}
 	}
 	ret = kv_enable_dpm_voltage_scaling(adev);
 	if (ret) {
-		DRM_ERROR("kv_enable_dpm_voltage_scaling failed\n");
+		drm_err(adev, "kv_enable_dpm_voltage_scaling failed\n");
 		return ret;
 	}
 	ret = kv_set_dpm_interval(adev);
 	if (ret) {
-		DRM_ERROR("kv_set_dpm_interval failed\n");
+		drm_err(adev, "kv_set_dpm_interval failed\n");
 		return ret;
 	}
 	ret = kv_set_dpm_boot_state(adev);
 	if (ret) {
-		DRM_ERROR("kv_set_dpm_boot_state failed\n");
+		drm_err(adev, "kv_set_dpm_boot_state failed\n");
 		return ret;
 	}
 	ret = kv_enable_ulv(adev, true);
 	if (ret) {
-		DRM_ERROR("kv_enable_ulv failed\n");
+		drm_err(adev, "kv_enable_ulv failed\n");
 		return ret;
 	}
 	kv_start_dpm(adev);
 	ret = kv_enable_didt(adev, true);
 	if (ret) {
-		DRM_ERROR("kv_enable_didt failed\n");
+		drm_err(adev, "kv_enable_didt failed\n");
 		return ret;
 	}
 	ret = kv_enable_smc_cac(adev, true);
 	if (ret) {
-		DRM_ERROR("kv_enable_smc_cac failed\n");
+		drm_err(adev, "kv_enable_smc_cac failed\n");
 		return ret;
 	}
 
@@ -1347,7 +1347,7 @@ static int kv_dpm_enable(struct amdgpu_device *adev)
 
 	ret = amdgpu_kv_smc_bapm_enable(adev, false);
 	if (ret) {
-		DRM_ERROR("amdgpu_kv_smc_bapm_enable failed\n");
+		drm_err(adev, "amdgpu_kv_smc_bapm_enable failed\n");
 		return ret;
 	}
 
@@ -1355,7 +1355,7 @@ static int kv_dpm_enable(struct amdgpu_device *adev)
 	    amdgpu_is_internal_thermal_sensor(adev->pm.int_thermal_type)) {
 		ret = kv_set_thermal_temperature_range(adev, KV_TEMP_RANGE_MIN, KV_TEMP_RANGE_MAX);
 		if (ret) {
-			DRM_ERROR("kv_set_thermal_temperature_range failed\n");
+			drm_err(adev, "kv_set_thermal_temperature_range failed\n");
 			return ret;
 		}
 		amdgpu_irq_get(adev, &adev->pm.dpm.thermal.irq,
@@ -1928,7 +1928,7 @@ static int kv_dpm_set_power_state(void *handle)
 	if (pi->bapm_enable) {
 		ret = amdgpu_kv_smc_bapm_enable(adev, adev->pm.ac_power);
 		if (ret) {
-			DRM_ERROR("amdgpu_kv_smc_bapm_enable failed\n");
+			drm_err(adev, "amdgpu_kv_smc_bapm_enable failed\n");
 			return ret;
 		}
 	}
@@ -1939,7 +1939,7 @@ static int kv_dpm_set_power_state(void *handle)
 			kv_update_dfs_bypass_settings(adev, new_ps);
 			ret = kv_calculate_ds_divider(adev);
 			if (ret) {
-				DRM_ERROR("kv_calculate_ds_divider failed\n");
+				drm_err(adev, "kv_calculate_ds_divider failed\n");
 				return ret;
 			}
 			kv_calculate_nbps_level_settings(adev);
@@ -1955,7 +1955,7 @@ static int kv_dpm_set_power_state(void *handle)
 
 			ret = kv_update_vce_dpm(adev, new_ps, old_ps);
 			if (ret) {
-				DRM_ERROR("kv_update_vce_dpm failed\n");
+				drm_err(adev, "kv_update_vce_dpm failed\n");
 				return ret;
 			}
 			kv_update_sclk_t(adev);
@@ -1968,7 +1968,7 @@ static int kv_dpm_set_power_state(void *handle)
 			kv_update_dfs_bypass_settings(adev, new_ps);
 			ret = kv_calculate_ds_divider(adev);
 			if (ret) {
-				DRM_ERROR("kv_calculate_ds_divider failed\n");
+				drm_err(adev, "kv_calculate_ds_divider failed\n");
 				return ret;
 			}
 			kv_calculate_nbps_level_settings(adev);
@@ -1980,7 +1980,7 @@ static int kv_dpm_set_power_state(void *handle)
 			kv_set_enabled_levels(adev);
 			ret = kv_update_vce_dpm(adev, new_ps, old_ps);
 			if (ret) {
-				DRM_ERROR("kv_update_vce_dpm failed\n");
+				drm_err(adev, "kv_update_vce_dpm failed\n");
 				return ret;
 			}
 			kv_update_acp_boot_level(adev);
@@ -2529,7 +2529,7 @@ static int kv_set_thermal_temperature_range(struct amdgpu_device *adev,
 	if (high_temp > max_temp)
 		high_temp = max_temp;
 	if (high_temp < low_temp) {
-		DRM_ERROR("invalid thermal range: %d - %d\n", low_temp, high_temp);
+		drm_err(adev, "invalid thermal range: %d - %d\n", low_temp, high_temp);
 		return -EINVAL;
 	}
 
@@ -2571,7 +2571,7 @@ static int kv_parse_sys_info_table(struct amdgpu_device *adev)
 					      data_offset);
 
 		if (crev != 8) {
-			DRM_ERROR("Unsupported IGP table: %d %d\n", frev, crev);
+			drm_err(adev, "Unsupported IGP table: %d %d\n", frev, crev);
 			return -EINVAL;
 		}
 		pi->sys_info.bootup_sclk = le32_to_cpu(igp_info->info_8.ulBootUpEngineClock);
@@ -2587,7 +2587,7 @@ static int kv_parse_sys_info_table(struct amdgpu_device *adev)
 		else
 			pi->sys_info.htc_hyst_lmt = igp_info->info_8.ucHtcHystLmt;
 		if (pi->sys_info.htc_tmp_lmt <= pi->sys_info.htc_hyst_lmt) {
-			DRM_ERROR("The htcTmpLmt should be larger than htcHystLmt.\n");
+			drm_err(adev, "The htcTmpLmt should be larger than htcHystLmt.\n");
 		}
 
 		if (le32_to_cpu(igp_info->info_8.ulSystemConfig) & (1 << 3))
@@ -3026,14 +3026,14 @@ static int kv_dpm_sw_init(void *handle)
 	if (amdgpu_dpm == 1)
 		amdgpu_pm_print_power_states(adev);
 	mutex_unlock(&adev->pm.mutex);
-	DRM_INFO("amdgpu: dpm initialized\n");
+	drm_info(adev, "amdgpu: dpm initialized\n");
 
 	return 0;
 
 dpm_failed:
 	kv_dpm_fini(adev);
 	mutex_unlock(&adev->pm.mutex);
-	DRM_ERROR("amdgpu: dpm initialization failed\n");
+	drm_err(adev, "amdgpu: dpm initialization failed\n");
 	return ret;
 }
 
@@ -3194,12 +3194,12 @@ static int kv_dpm_process_interrupt(struct amdgpu_device *adev,
 
 	switch (entry->src_id) {
 	case 230: /* thermal low to high */
-		DRM_DEBUG("IH: thermal low to high\n");
+		drm_dbg(adev, "IH: thermal low to high\n");
 		adev->pm.dpm.thermal.high_to_low = false;
 		queue_thermal = true;
 		break;
 	case 231: /* thermal high to low */
-		DRM_DEBUG("IH: thermal high to low\n");
+		drm_dbg(adev, "IH: thermal high to low\n");
 		adev->pm.dpm.thermal.high_to_low = true;
 		queue_thermal = true;
 		break;
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
index c00ba4b23c9a..8ba673ca2f5e 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
@@ -1932,7 +1932,7 @@ static void si_update_dte_from_pl2(struct amdgpu_device *adev,
 			dte_data->tdep_r[i] = dte_data->r[4];
 		}
 	} else {
-		DRM_ERROR("Invalid PL2! DTE will not be updated.\n");
+		drm_err(adev, "Invalid PL2! DTE will not be updated.\n");
 	}
 }
 
@@ -1994,7 +1994,7 @@ static void si_initialize_powertune_defaults(struct amdgpu_device *adev)
 			break;
 		default:
 			if (si_pi->dte_data.enable_dte_by_default == true)
-				DRM_ERROR("DTE is not enabled!\n");
+				drm_err(adev, "DTE is not enabled!\n");
 			break;
 		}
 	} else if (adev->asic_type == CHIP_PITCAIRN) {
@@ -2122,7 +2122,7 @@ static void si_initialize_powertune_defaults(struct amdgpu_device *adev)
 		si_pi->dte_data = dte_data_sun_xt;
 		update_dte_from_pl2 = true;
 	} else {
-		DRM_ERROR("Unknown SI asic revision, failed to initialize PowerTune!\n");
+		drm_err(adev, "Unknown SI asic revision, failed to initialize PowerTune!\n");
 		return;
 	}
 
@@ -6336,15 +6336,15 @@ static int si_patch_dependency_tables_based_on_leakage(struct amdgpu_device *ade
 	ret = si_patch_single_dependency_table_based_on_leakage(adev,
 								&adev->pm.dpm.dyn_state.vddc_dependency_on_sclk);
 	if (ret)
-		DRM_ERROR("Could not patch vddc_on_sclk leakage table\n");
+		drm_err(adev, "Could not patch vddc_on_sclk leakage table\n");
 	ret = si_patch_single_dependency_table_based_on_leakage(adev,
 								&adev->pm.dpm.dyn_state.vddc_dependency_on_mclk);
 	if (ret)
-		DRM_ERROR("Could not patch vddc_on_mclk leakage table\n");
+		drm_err(adev, "Could not patch vddc_on_mclk leakage table\n");
 	ret = si_patch_single_dependency_table_based_on_leakage(adev,
 								&adev->pm.dpm.dyn_state.vddci_dependency_on_mclk);
 	if (ret)
-		DRM_ERROR("Could not patch vddci_on_mclk leakage table\n");
+		drm_err(adev, "Could not patch vddci_on_mclk leakage table\n");
 	return ret;
 }
 
@@ -6405,7 +6405,7 @@ static int si_thermal_set_temperature_range(struct amdgpu_device *adev,
 	if (high_temp > max_temp)
 		high_temp = max_temp;
 	if (high_temp < low_temp) {
-		DRM_ERROR("invalid thermal range: %d - %d\n", low_temp, high_temp);
+		drm_err(adev, "invalid thermal range: %d - %d\n", low_temp, high_temp);
 		return -EINVAL;
 	}
 
@@ -6503,7 +6503,7 @@ static int si_thermal_setup_fan_table(struct amdgpu_device *adev)
 					  si_pi->sram_end);
 
 	if (ret) {
-		DRM_ERROR("Failed to load fan table to the SMC.");
+		drm_err(adev, "Failed to load fan table to the SMC.");
 		adev->pm.dpm.fan.ucode_fan_control = false;
 	}
 
@@ -6774,7 +6774,7 @@ static int si_dpm_enable(struct amdgpu_device *adev)
 	if (pi->voltage_control || si_pi->voltage_control_svi2) {
 		ret = si_construct_voltage_tables(adev);
 		if (ret) {
-			DRM_ERROR("si_construct_voltage_tables failed\n");
+			drm_err(adev, "si_construct_voltage_tables failed\n");
 			return ret;
 		}
 	}
@@ -6796,64 +6796,64 @@ static int si_dpm_enable(struct amdgpu_device *adev)
 	si_program_vc(adev);
 	ret = si_upload_firmware(adev);
 	if (ret) {
-		DRM_ERROR("si_upload_firmware failed\n");
+		drm_err(adev, "si_upload_firmware failed\n");
 		return ret;
 	}
 	ret = si_process_firmware_header(adev);
 	if (ret) {
-		DRM_ERROR("si_process_firmware_header failed\n");
+		drm_err(adev, "si_process_firmware_header failed\n");
 		return ret;
 	}
 	ret = si_initial_switch_from_arb_f0_to_f1(adev);
 	if (ret) {
-		DRM_ERROR("si_initial_switch_from_arb_f0_to_f1 failed\n");
+		drm_err(adev, "si_initial_switch_from_arb_f0_to_f1 failed\n");
 		return ret;
 	}
 	ret = si_init_smc_table(adev);
 	if (ret) {
-		DRM_ERROR("si_init_smc_table failed\n");
+		drm_err(adev, "si_init_smc_table failed\n");
 		return ret;
 	}
 	ret = si_init_smc_spll_table(adev);
 	if (ret) {
-		DRM_ERROR("si_init_smc_spll_table failed\n");
+		drm_err(adev, "si_init_smc_spll_table failed\n");
 		return ret;
 	}
 	ret = si_init_arb_table_index(adev);
 	if (ret) {
-		DRM_ERROR("si_init_arb_table_index failed\n");
+		drm_err(adev, "si_init_arb_table_index failed\n");
 		return ret;
 	}
 	if (eg_pi->dynamic_ac_timing) {
 		ret = si_populate_mc_reg_table(adev, boot_ps);
 		if (ret) {
-			DRM_ERROR("si_populate_mc_reg_table failed\n");
+			drm_err(adev, "si_populate_mc_reg_table failed\n");
 			return ret;
 		}
 	}
 	ret = si_initialize_smc_cac_tables(adev);
 	if (ret) {
-		DRM_ERROR("si_initialize_smc_cac_tables failed\n");
+		drm_err(adev, "si_initialize_smc_cac_tables failed\n");
 		return ret;
 	}
 	ret = si_initialize_hardware_cac_manager(adev);
 	if (ret) {
-		DRM_ERROR("si_initialize_hardware_cac_manager failed\n");
+		drm_err(adev, "si_initialize_hardware_cac_manager failed\n");
 		return ret;
 	}
 	ret = si_initialize_smc_dte_tables(adev);
 	if (ret) {
-		DRM_ERROR("si_initialize_smc_dte_tables failed\n");
+		drm_err(adev, "si_initialize_smc_dte_tables failed\n");
 		return ret;
 	}
 	ret = si_populate_smc_tdp_limits(adev, boot_ps);
 	if (ret) {
-		DRM_ERROR("si_populate_smc_tdp_limits failed\n");
+		drm_err(adev, "si_populate_smc_tdp_limits failed\n");
 		return ret;
 	}
 	ret = si_populate_smc_tdp_limits_2(adev, boot_ps);
 	if (ret) {
-		DRM_ERROR("si_populate_smc_tdp_limits_2 failed\n");
+		drm_err(adev, "si_populate_smc_tdp_limits_2 failed\n");
 		return ret;
 	}
 	si_program_response_times(adev);
@@ -6861,7 +6861,7 @@ static int si_dpm_enable(struct amdgpu_device *adev)
 	si_dpm_start_smc(adev);
 	ret = si_notify_smc_display_change(adev, false);
 	if (ret) {
-		DRM_ERROR("si_notify_smc_display_change failed\n");
+		drm_err(adev, "si_notify_smc_display_change failed\n");
 		return ret;
 	}
 	si_enable_sclk_control(adev, true);
@@ -6963,12 +6963,12 @@ static int si_dpm_set_power_state(void *handle)
 
 	ret = si_disable_ulv(adev);
 	if (ret) {
-		DRM_ERROR("si_disable_ulv failed\n");
+		drm_err(adev, "si_disable_ulv failed\n");
 		return ret;
 	}
 	ret = si_restrict_performance_levels_before_switch(adev);
 	if (ret) {
-		DRM_ERROR("si_restrict_performance_levels_before_switch failed\n");
+		drm_err(adev, "si_restrict_performance_levels_before_switch failed\n");
 		return ret;
 	}
 	if (eg_pi->pcie_performance_request)
@@ -6976,56 +6976,56 @@ static int si_dpm_set_power_state(void *handle)
 	ni_set_uvd_clock_before_set_eng_clock(adev, new_ps, old_ps);
 	ret = si_enable_power_containment(adev, new_ps, false);
 	if (ret) {
-		DRM_ERROR("si_enable_power_containment failed\n");
+		drm_err(adev, "si_enable_power_containment failed\n");
 		return ret;
 	}
 	ret = si_enable_smc_cac(adev, new_ps, false);
 	if (ret) {
-		DRM_ERROR("si_enable_smc_cac failed\n");
+		drm_err(adev, "si_enable_smc_cac failed\n");
 		return ret;
 	}
 	ret = si_halt_smc(adev);
 	if (ret) {
-		DRM_ERROR("si_halt_smc failed\n");
+		drm_err(adev, "si_halt_smc failed\n");
 		return ret;
 	}
 	ret = si_upload_sw_state(adev, new_ps);
 	if (ret) {
-		DRM_ERROR("si_upload_sw_state failed\n");
+		drm_err(adev, "si_upload_sw_state failed\n");
 		return ret;
 	}
 	ret = si_upload_smc_data(adev);
 	if (ret) {
-		DRM_ERROR("si_upload_smc_data failed\n");
+		drm_err(adev, "si_upload_smc_data failed\n");
 		return ret;
 	}
 	ret = si_upload_ulv_state(adev);
 	if (ret) {
-		DRM_ERROR("si_upload_ulv_state failed\n");
+		drm_err(adev, "si_upload_ulv_state failed\n");
 		return ret;
 	}
 	if (eg_pi->dynamic_ac_timing) {
 		ret = si_upload_mc_reg_table(adev, new_ps);
 		if (ret) {
-			DRM_ERROR("si_upload_mc_reg_table failed\n");
+			drm_err(adev, "si_upload_mc_reg_table failed\n");
 			return ret;
 		}
 	}
 	ret = si_program_memory_timing_parameters(adev, new_ps);
 	if (ret) {
-		DRM_ERROR("si_program_memory_timing_parameters failed\n");
+		drm_err(adev, "si_program_memory_timing_parameters failed\n");
 		return ret;
 	}
 	si_set_pcie_lane_width_in_smc(adev, new_ps, old_ps);
 
 	ret = si_resume_smc(adev);
 	if (ret) {
-		DRM_ERROR("si_resume_smc failed\n");
+		drm_err(adev, "si_resume_smc failed\n");
 		return ret;
 	}
 	ret = si_set_sw_state(adev);
 	if (ret) {
-		DRM_ERROR("si_set_sw_state failed\n");
+		drm_err(adev, "si_set_sw_state failed\n");
 		return ret;
 	}
 	ni_set_uvd_clock_after_set_eng_clock(adev, new_ps, old_ps);
@@ -7033,23 +7033,23 @@ static int si_dpm_set_power_state(void *handle)
 		si_notify_link_speed_change_after_state_change(adev, new_ps, old_ps);
 	ret = si_set_power_state_conditionally_enable_ulv(adev, new_ps);
 	if (ret) {
-		DRM_ERROR("si_set_power_state_conditionally_enable_ulv failed\n");
+		drm_err(adev, "si_set_power_state_conditionally_enable_ulv failed\n");
 		return ret;
 	}
 	ret = si_enable_smc_cac(adev, new_ps, true);
 	if (ret) {
-		DRM_ERROR("si_enable_smc_cac failed\n");
+		drm_err(adev, "si_enable_smc_cac failed\n");
 		return ret;
 	}
 	ret = si_enable_power_containment(adev, new_ps, true);
 	if (ret) {
-		DRM_ERROR("si_enable_power_containment failed\n");
+		drm_err(adev, "si_enable_power_containment failed\n");
 		return ret;
 	}
 
 	ret = si_power_control_set_level(adev);
 	if (ret) {
-		DRM_ERROR("si_power_control_set_level failed\n");
+		drm_err(adev, "si_power_control_set_level failed\n");
 		return ret;
 	}
 
@@ -7655,7 +7655,7 @@ static int si_dpm_init_microcode(struct amdgpu_device *adev)
 
 out:
 	if (err) {
-		DRM_ERROR("si_smc: Failed to load firmware. err = %d\"%s\"\n",
+		drm_err(adev, "si_smc: Failed to load firmware. err = %d\"%s\"\n",
 			  err, fw_name);
 		release_firmware(adev->pm.fw);
 		adev->pm.fw = NULL;
@@ -7703,14 +7703,14 @@ static int si_dpm_sw_init(void *handle)
 	if (amdgpu_dpm == 1)
 		amdgpu_pm_print_power_states(adev);
 	mutex_unlock(&adev->pm.mutex);
-	DRM_INFO("amdgpu: dpm initialized\n");
+	drm_info(adev, "amdgpu: dpm initialized\n");
 
 	return 0;
 
 dpm_failed:
 	si_dpm_fini(adev);
 	mutex_unlock(&adev->pm.mutex);
-	DRM_ERROR("amdgpu: dpm initialization failed\n");
+	drm_err(adev, "amdgpu: dpm initialization failed\n");
 	return ret;
 }
 
-- 
2.27.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 2/3] drm/amdgpu: Inform user about effect of running without DPM

[Paul Menzel]

Currently, most users have no idea about the effects of failed DPM
initialization. So add it to the log message.

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/si_dpm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
index b179bdc17cdc..f054ded902f2 100644
--- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
@@ -3033,7 +3033,7 @@ static int kv_dpm_sw_init(void *handle)
 dpm_failed:
 	kv_dpm_fini(adev);
 	mutex_unlock(&adev->pm.mutex);
-	drm_err(adev, "amdgpu: dpm initialization failed\n");
+	drm_err(adev, "amdgpu: dpm initialization failed. Clocks set up by firmware will be used. Most likely they are low, so performance might suffer.\n");
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
index 8ba673ca2f5e..f7edc1d50df4 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
@@ -7710,7 +7710,7 @@ static int si_dpm_sw_init(void *handle)
 dpm_failed:
 	si_dpm_fini(adev);
 	mutex_unlock(&adev->pm.mutex);
-	drm_err(adev, "amdgpu: dpm initialization failed\n");
+	drm_err(adev, "amdgpu: dpm initialization failed. Clocks set up by firmware will be used. Most likely they are low, so performance might suffer.\n");
 	return ret;
 }
 
-- 
2.27.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2 3/3] drm/amdgpu: Warn about disabled DPM

[Paul Menzel]

Currently, besides there is no explicit message, that DPM is disabled.
The user would need to know, that the missing success line indicates
that.

    [drm] amdgpu: dpm initialized

So, add an explicit message, and make it log level warning, as disabling
dpm is not the default, and device performance will most likely suffer.

Resolves: https://gitlab.freedesktop.org/drm/amd/-/issues/1173
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
v2: Use new print helpers, and inform user about effects.

 drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 4 +++-
 drivers/gpu/drm/amd/amdgpu/si_dpm.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
index f054ded902f2..c601587c6d59 100644
--- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
@@ -3014,8 +3014,10 @@ static int kv_dpm_sw_init(void *handle)
 	adev->pm.current_mclk = adev->clock.default_mclk;
 	adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
 
-	if (amdgpu_dpm == 0)
+	if (amdgpu_dpm == 0) {
+		drm_warn(adev, "amdgpu: dpm disabled per parameter. Your graphics device will run with lower clocks impacting graphics performance.\n");
 		return 0;
+	}
 
 	INIT_WORK(&adev->pm.dpm.thermal.work, amdgpu_dpm_thermal_work_handler);
 	mutex_lock(&adev->pm.mutex);
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
index f7edc1d50df4..1f35d5a36300 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
@@ -7687,8 +7687,10 @@ static int si_dpm_sw_init(void *handle)
 	adev->pm.current_mclk = adev->clock.default_mclk;
 	adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
 
-	if (amdgpu_dpm == 0)
+	if (amdgpu_dpm == 0) {
+		drm_warn(adev, "amdgpu: dpm disabled per parameter. Your graphics device will run with lower clocks impacting graphics performance.\n");
 		return 0;
+	}
 
 	ret = si_dpm_init_microcode(adev);
 	if (ret)
-- 
2.27.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 3/3] drm/amdgpu: Warn about disabled DPM

[Christian König]

Am 19.06.20 um 20:50 schrieb Paul Menzel:
> Currently, besides there is no explicit message, that DPM is disabled.
> The user would need to know, that the missing success line indicates
> that.
>
>      [drm] amdgpu: dpm initialized
>
> So, add an explicit message, and make it log level warning, as disabling
> dpm is not the default, and device performance will most likely suffer.
>
> Resolves: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1173&data=02%7C01%7Cchristian.koenig%40amd.com%7Ca93f6121c2ab464b7c1908d81481a24a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637281894303394855&sdata=lbcpB8H9TnbISG8VFfciKh%2FKGC7YwkVYxUf4Y7dDVt8%3D&reserved=0
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
> v2: Use new print helpers, and inform user about effects.
>
>   drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/si_dpm.c | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
> index f054ded902f2..c601587c6d59 100644
> --- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
> @@ -3014,8 +3014,10 @@ static int kv_dpm_sw_init(void *handle)
>   	adev->pm.current_mclk = adev->clock.default_mclk;
>   	adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>   
> -	if (amdgpu_dpm == 0)
> +	if (amdgpu_dpm == 0) {
> +		drm_warn(adev, "amdgpu: dpm disabled per parameter. Your graphics device will run with lower clocks impacting graphics performance.\n");

I'm not very keen about this. When an user specifies that DPM shouldn't 
be used the driver doesn't need to inform the user about this once more.

In other words shooting in your own foot is supposed to hurt.

Christian.

>   		return 0;
> +	}
>   
>   	INIT_WORK(&adev->pm.dpm.thermal.work, amdgpu_dpm_thermal_work_handler);
>   	mutex_lock(&adev->pm.mutex);
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
> index f7edc1d50df4..1f35d5a36300 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
> @@ -7687,8 +7687,10 @@ static int si_dpm_sw_init(void *handle)
>   	adev->pm.current_mclk = adev->clock.default_mclk;
>   	adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>   
> -	if (amdgpu_dpm == 0)
> +	if (amdgpu_dpm == 0) {
> +		drm_warn(adev, "amdgpu: dpm disabled per parameter. Your graphics device will run with lower clocks impacting graphics performance.\n");
>   		return 0;
> +	}
>   
>   	ret = si_dpm_init_microcode(adev);
>   	if (ret)

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 3/3] drm/amdgpu: Warn about disabled DPM

[Paul Menzel]

Dear Christian,


Am 22.06.20 um 15:39 schrieb Christian König:
> Am 19.06.20 um 20:50 schrieb Paul Menzel:
>> Currently, besides there is no explicit message, that DPM is disabled.
>> The user would need to know, that the missing success line indicates
>> that.
>>
>>      [drm] amdgpu: dpm initialized
>>
>> So, add an explicit message, and make it log level warning, as disabling
>> dpm is not the default, and device performance will most likely suffer.
>>
>> Resolves: 
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1173&data=02%7C01%7Cchristian.koenig%40amd.com%7Ca93f6121c2ab464b7c1908d81481a24a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637281894303394855&sdata=lbcpB8H9TnbISG8VFfciKh%2FKGC7YwkVYxUf4Y7dDVt8%3D&reserved=0 

That URL is not mine. ;-)

>> Cc: amd-gfx@lists.freedesktop.org
>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> ---
>> v2: Use new print helpers, and inform user about effects.
>>
>>   drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 4 +++-
>>   drivers/gpu/drm/amd/amdgpu/si_dpm.c | 4 +++-
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c 
>> b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>> index f054ded902f2..c601587c6d59 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>> @@ -3014,8 +3014,10 @@ static int kv_dpm_sw_init(void *handle)
>>       adev->pm.current_mclk = adev->clock.default_mclk;
>>       adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>> -    if (amdgpu_dpm == 0)
>> +    if (amdgpu_dpm == 0) {
>> +        drm_warn(adev, "amdgpu: dpm disabled per parameter. Your 
>> graphics device will run with lower clocks impacting graphics 
>> performance.\n");
> 
> I'm not very keen about this. When an user specifies that DPM shouldn't 
> be used the driver doesn't need to inform the user about this once more.
> 
> In other words shooting in your own foot is supposed to hurt.

Maybe. The other point of view is, how does having the clarity in the 
logs hurt? For example, if the user added the parameter intentionally, 
maybe they made a typo, and it’s actually not applied. Or there is a bug 
in the parameter handling. Having explicit log messages is good in my 
opinion.

Secondly, the parameter could have been left there unintentionally. 
Having the message in the logs, makes the user aware of tha.


Kind regards,

Paul


>>           return 0;
>> +    }
>>       INIT_WORK(&adev->pm.dpm.thermal.work, 
>> amdgpu_dpm_thermal_work_handler);
>>       mutex_lock(&adev->pm.mutex);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c 
>> b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>> index f7edc1d50df4..1f35d5a36300 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>> @@ -7687,8 +7687,10 @@ static int si_dpm_sw_init(void *handle)
>>       adev->pm.current_mclk = adev->clock.default_mclk;
>>       adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>> -    if (amdgpu_dpm == 0)
>> +    if (amdgpu_dpm == 0) {
>> +        drm_warn(adev, "amdgpu: dpm disabled per parameter. Your 
>> graphics device will run with lower clocks impacting graphics 
>> performance.\n");
>>           return 0;
>> +    }
>>       ret = si_dpm_init_microcode(adev);
>>       if (ret)
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 3/3] drm/amdgpu: Warn about disabled DPM

[Christian König]

Am 22.06.20 um 19:25 schrieb Paul Menzel:
> Dear Christian,
>
>
> Am 22.06.20 um 15:39 schrieb Christian König:
>> Am 19.06.20 um 20:50 schrieb Paul Menzel:
>>> Currently, besides there is no explicit message, that DPM is disabled.
>>> The user would need to know, that the missing success line indicates
>>> that.
>>>
>>>      [drm] amdgpu: dpm initialized
>>>
>>> So, add an explicit message, and make it log level warning, as 
>>> disabling
>>> dpm is not the default, and device performance will most likely suffer.
>>>
>>> Resolves: 
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1173&data=02%7C01%7Cchristian.koenig%40amd.com%7C72d0b71d439e46d6253f08d816d150c6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637284435558396492&sdata=5EXY1o1zwXJRzN9fqpUg%2BQNJGB3zAlWKnGWsdFXRcjA%3D&reserved=0 
>>
>
> That URL is not mine. ;-)
>
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>> ---
>>> v2: Use new print helpers, and inform user about effects.
>>>
>>>   drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 4 +++-
>>>   drivers/gpu/drm/amd/amdgpu/si_dpm.c | 4 +++-
>>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>> index f054ded902f2..c601587c6d59 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>> @@ -3014,8 +3014,10 @@ static int kv_dpm_sw_init(void *handle)
>>>       adev->pm.current_mclk = adev->clock.default_mclk;
>>>       adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>>> -    if (amdgpu_dpm == 0)
>>> +    if (amdgpu_dpm == 0) {
>>> +        drm_warn(adev, "amdgpu: dpm disabled per parameter. Your 
>>> graphics device will run with lower clocks impacting graphics 
>>> performance.\n");
>>
>> I'm not very keen about this. When an user specifies that DPM 
>> shouldn't be used the driver doesn't need to inform the user about 
>> this once more.
>>
>> In other words shooting in your own foot is supposed to hurt.
>
> Maybe. The other point of view is, how does having the clarity in the 
> logs hurt?

Well, you are spamming the logs with a warning about an intentional 
behavior.

> For example, if the user added the parameter intentionally, maybe they 
> made a typo, and it’s actually not applied. Or there is a bug in the 
> parameter handling. Having explicit log messages is good in my opinion.
>
> Secondly, the parameter could have been left there unintentionally. 
> Having the message in the logs, makes the user aware of tha.

And exactly for this reason the kernel command line is printed as the 
second line of the logs.

Duplicating this in each driver is not only overkill, but also very 
error prone.

Sorry, but this is absolutely don't think that this is a good idea.

Regards,
Christian.

>
>
> Kind regards,
>
> Paul
>
>
>>>           return 0;
>>> +    }
>>>       INIT_WORK(&adev->pm.dpm.thermal.work, 
>>> amdgpu_dpm_thermal_work_handler);
>>>       mutex_lock(&adev->pm.mutex);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>> index f7edc1d50df4..1f35d5a36300 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>> @@ -7687,8 +7687,10 @@ static int si_dpm_sw_init(void *handle)
>>>       adev->pm.current_mclk = adev->clock.default_mclk;
>>>       adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>>> -    if (amdgpu_dpm == 0)
>>> +    if (amdgpu_dpm == 0) {
>>> +        drm_warn(adev, "amdgpu: dpm disabled per parameter. Your 
>>> graphics device will run with lower clocks impacting graphics 
>>> performance.\n");
>>>           return 0;
>>> +    }
>>>       ret = si_dpm_init_microcode(adev);
>>>       if (ret)
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 3/3] drm/amdgpu: Warn about disabled DPM

[Paul Menzel]

Dear Christian,


Am 22.06.20 um 19:41 schrieb Christian König:
> Am 22.06.20 um 19:25 schrieb Paul Menzel:
>> Am 22.06.20 um 15:39 schrieb Christian König:
>>> Am 19.06.20 um 20:50 schrieb Paul Menzel:
>>>> Currently, besides there is no explicit message, that DPM is disabled.
>>>> The user would need to know, that the missing success line indicates
>>>> that.
>>>>
>>>>      [drm] amdgpu: dpm initialized
>>>>
>>>> So, add an explicit message, and make it log level warning, as 
>>>> disabling
>>>> dpm is not the default, and device performance will most likely suffer.
>>>>
>>>> Resolves: 
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1173&data=02%7C01%7Cchristian.koenig%40amd.com%7C72d0b71d439e46d6253f08d816d150c6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637284435558396492&sdata=5EXY1o1zwXJRzN9fqpUg%2BQNJGB3zAlWKnGWsdFXRcjA%3D&reserved=0
>>
>> That URL is not mine. ;-)
>>
>>>> Cc: amd-gfx@lists.freedesktop.org
>>>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>>> ---
>>>> v2: Use new print helpers, and inform user about effects.
>>>>
>>>>   drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 4 +++-
>>>>   drivers/gpu/drm/amd/amdgpu/si_dpm.c | 4 +++-
>>>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>>> index f054ded902f2..c601587c6d59 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>>> @@ -3014,8 +3014,10 @@ static int kv_dpm_sw_init(void *handle)
>>>>       adev->pm.current_mclk = adev->clock.default_mclk;
>>>>       adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>>>> -    if (amdgpu_dpm == 0)
>>>> +    if (amdgpu_dpm == 0) {
>>>> +        drm_warn(adev, "amdgpu: dpm disabled per parameter. Your 
>>>> graphics device will run with lower clocks impacting graphics 
>>>> performance.\n");
>>>
>>> I'm not very keen about this. When an user specifies that DPM 
>>> shouldn't be used the driver doesn't need to inform the user about 
>>> this once more.
>>>
>>> In other words shooting in your own foot is supposed to hurt.
>>
>> Maybe. The other point of view is, how does having the clarity in the 
>> logs hurt?
> 
> Well, you are spamming the logs with a warning about an intentional 
> behavior.
> 
>> For example, if the user added the parameter intentionally, maybe they 
>> made a typo, and it’s actually not applied. Or there is a bug in the 
>> parameter handling. Having explicit log messages is good in my opinion.
>>
>> Secondly, the parameter could have been left there unintentionally. 
>> Having the message in the logs, makes the user aware of that.
> 
> And exactly for this reason the kernel command line is printed as the 
> second line of the logs.

No, I disagree. Having the parameters listed, and knowing the actual 
effects are two totally different things. It’s user interface design 101 
to give clear feedback. I think, you are looking through the glasses of 
a developer, and not of a non-developer.

> Duplicating this in each driver is not only overkill, but also very 
> error prone.

What drivers do you mean, and what is error prone?

> Sorry, but this is absolutely don't think that this is a good idea.

Despite other subsystems actually “spamming” the logs for parameters 
given on the command line, here is my motivation.

Unfortunately, there is a serious issue with first(?) generation Ryzen 
CPUs, and maybe even external AMD graphics cards [1]. In comment 29 it 
is suggested to use `amdgpu.dpm=0` [1], and further it’s said, it’s 
disabling power management. Reading the comments further [2], the user 
gets the impression turning off power management will cause the device 
to be operated at the highest performance [3].

Trying this option, I was surprised of the degraded performance, and 
only in #radeon@irc.freenode.net my question was answered, saying that 
it’s actually depending on the system firmware how clocks are set up, 
and it’s common to use the *lowest* clocks. So the warning message, 
would have helped me a lot.

So, especially with these option used by “normal” users, having clear 
feedback on specified option, especially those decreasing performance, 
is very important.


Kind regards,

Paul


>>>>           return 0;
>>>> +    }
>>>>       INIT_WORK(&adev->pm.dpm.thermal.work, 
>>>> amdgpu_dpm_thermal_work_handler);
>>>>       mutex_lock(&adev->pm.mutex);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>>> index f7edc1d50df4..1f35d5a36300 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>>> @@ -7687,8 +7687,10 @@ static int si_dpm_sw_init(void *handle)
>>>>       adev->pm.current_mclk = adev->clock.default_mclk;
>>>>       adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>>>> -    if (amdgpu_dpm == 0)
>>>> +    if (amdgpu_dpm == 0) {
>>>> +        drm_warn(adev, "amdgpu: dpm disabled per parameter. Your 
>>>> graphics device will run with lower clocks impacting graphics 
>>>> performance.\n");
>>>>           return 0;
>>>> +    }
>>>>       ret = si_dpm_init_microcode(adev);
>>>>       if (ret)



[1]: https://bugzilla.kernel.org/show_bug.cgi?id=206903#c29
[2]: https://bugzilla.kernel.org/show_bug.cgi?id=206903#c33
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 3/3] drm/amdgpu: Warn about disabled DPM

[Christian König]

Am 22.06.20 um 23:41 schrieb Paul Menzel:
> [SNIP]
>>> For example, if the user added the parameter intentionally, maybe 
>>> they made a typo, and it’s actually not applied. Or there is a bug 
>>> in the parameter handling. Having explicit log messages is good in 
>>> my opinion.
>>>
>>> Secondly, the parameter could have been left there unintentionally. 
>>> Having the message in the logs, makes the user aware of that.
>>
>> And exactly for this reason the kernel command line is printed as the 
>> second line of the logs.
>
> No, I disagree. Having the parameters listed, and knowing the actual 
> effects are two totally different things. It’s user interface design 
> 101 to give clear feedback. I think, you are looking through the 
> glasses of a developer, and not of a non-developer.

Well my job is to see this as a maintainer and keep the driver simple 
and clean so that we can easily maintain and extend it, but at the same 
time it should obviously give the best experience to end users.

So I'm certainly weighting this, but currently I don't see a good 
argument to add something like this.

See essentially you are requesting that an intended behavior should give 
a warning. That is like telling the user to not do something he wants.

If an user doesn't knew what he is doing, that's obviously a big problem 
as well. But I would say in this case we should improve the documentation.

Adding a warning would be justified for options like experimental 
features which could damage the hardware by going beyond the device 
capabilities for example.

>> Duplicating this in each driver is not only overkill, but also very 
>> error prone.
>
> What drivers do you mean, and what is error prone?

Individual hardware drivers in general.

>
>> Sorry, but this is absolutely don't think that this is a good idea.
>
> Despite other subsystems actually “spamming” the logs for parameters 
> given on the command line, here is my motivation.
>
> Unfortunately, there is a serious issue with first(?) generation Ryzen 
> CPUs, and maybe even external AMD graphics cards [1]. In comment 29 it 
> is suggested to use `amdgpu.dpm=0` [1], and further it’s said, it’s 
> disabling power management. Reading the comments further [2], the user 
> gets the impression turning off power management will cause the device 
> to be operated at the highest performance [3].
>
> Trying this option, I was surprised of the degraded performance, and 
> only in #radeon@irc.freenode.net my question was answered, saying that 
> it’s actually depending on the system firmware how clocks are set up, 
> and it’s common to use the *lowest* clocks. So the warning message, 
> would have helped me a lot.

Yeah ok, but that hardware falls back to the lowest possible performance 
setting as safety precaution when turn of power management is common 
knowledge.

In other words you are writing on the cup of coffee "Beware content is hot".

>
> So, especially with these option used by “normal” users, having clear 
> feedback on specified option, especially those decreasing performance, 
> is very important.

Well, what we could do is improve the documentation of the module option.

Regards,
Christian.

>
>
> Kind regards,
>
> Paul

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx