Hi,
this bug: https://bugs.freedesktop.org/show_bug.cgi?id=108940#add_comment has annoyed a lot of people...
I'm neither an C programmer nor an "expert" in driver programming, so my "solution" is based on experience in other programming languages...
My code is - I guess - crappy as hell, sorry for that. My mainboard is an AsRock B450M Pro4, with latest BIOS P3.10 03/07/2019 and an AMD Ryzen 5 2400G.
When looking at the dmesg output, I noticed that reading the clock values in function dcn_bw_update_from_pplib changed from kernel version to kernel version.
In 5.1+, both FCLK and DCFCLK are printed correctly.
In 5.0, FCLK is printed correctly, DCFCLK is printed as "Invalid clock".
In < 5.0, only DCFCLK is printed (as "Invalid clock")
--
5.0.7:
DM_PPLIB: values for Invalid clock
DM_PPLIB: 0 in kHz
DM_PPLIB: 0 in kHz
DM_PPLIB: 0 in kHz
DM_PPLIB: 1600000 in kHz
WARNING: CPU: 2 PID: 276 at drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c:1380 dcn_bw_update_from_pplib+0x171/0x290 [amdgpu]
RIP: 0010:dcn_bw_update_from_pplib+0x171/0x290 [amdgpu]
DM_PPLIB: values for Invalid clock
DM_PPLIB: 300000 in kHz
DM_PPLIB: 600000 in kHz
DM_PPLIB: 626000 in kHz
DM_PPLIB: 654000 in kHz
--
5.1_rc3
DM_PPLIB: values for F clock
DM_PPLIB: 0 in kHz
DM_PPLIB: 0 in kHz
DM_PPLIB: 0 in kHz
DM_PPLIB: 1600000 in kHz
WARNING: CPU: 6 PID: 292 at drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c:1373 dcn_bw_update_from_pplib+0x174/0x2a0 [amdgpu]
RIP: 0010:dcn_bw_update_from_pplib+0x174/0x2a0 [amdgpu]
DM_PPLIB: values for DCF clock
DM_PPLIB: 300000 in kHz
DM_PPLIB: 600000 in kHz
DM_PPLIB: 626000 in kHz
DM_PPLIB: 654000 in kHz
--
After a bit of try and error, I found out that deep in the bios is an option to set the "System configuration" to 6 possible values and that this relates to the cTDP settings.
(3 Commercial, 3 Customer).
Each of these settings lead to different results - depending on the number of levels set.
With the 45W TDP commercial setting, I got the following F clock results:
DM_PPLIB: values for F clock
DM_PPLIB: 0 in kHz
DM_PPLIB: 400000 in kHz
DM_PPLIB: 933000 in kHz
DM_PPLIB: 1067000 in kHz
Then, I took a deeper look at the source code - and changed the validation in verify_clock_values (drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c)
Before, every khz setting == 0 was regarded as an "fatal" error, leading to no possible initialization.
I guess that this isn't entirely wrong as 0 makes no sense. But in my case - and when I understood the code correctly - it's not entirely right, too, as there are some levels that could be used.
My assumption is that the BIOS should provide these values in a fixed order, but (as I even had to change bios settings to just get the basic clock values right) - the ASRock BIOS simply does nothing right...
>From my understanding, the previous code in dcn_bw_update_from_pplib is already based on the assumption that there is no fixed number of levels provided, but that there must be at least 3 and a maximum of 4 levels provided.
Instead of taking the results directly from the "dm_pp_clock_levels_with_voltage" struct, I gathered the valid clock KHz values and converted them to hertz in the validation function, returning the clocks as an array. An additional parameter to the function contains the actual (not maximum) number of correct values gathered.
This fixes the initialization and removes the annoying BREAK_TO_DEBUGGER output.
I think it is still not correct, as the FCLK values are not provided by the BIOS - and the approach to just bail out, because something is not provided by the bios, seems wrong to me.
There is a comment in the function that suggests that there is a better way, which sounds more sane to me:
/* TODO: This is not the proper way to obtain fabric_and_dram_bandwidth, should be min(fclk, memclk) */
Is memclk DFCLK? Or how can one get the memclk values?
I hope that someone could fix this properly or at least verify that my assumptions are correct - so I can maybe work out a better patch.
Thanks in advance,
Henning
diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dcn_calcs.c b/drivers/gpu/drm/amd/display/dc/calcs/dcn_calcs.c
index eb62d10bb65c..d97c060e8b99 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/dcn_calcs.c
+++ b/drivers/gpu/drm/amd/display/dc/calcs/dcn_calcs.c
@@ -1327,19 +1327,26 @@ unsigned int dcn_find_dcfclk_suits_all(
return dcf_clk;
}
-static bool verify_clock_values(struct dm_pp_clock_levels_with_voltage *clks)
+static float* verify_clock_values(struct dm_pp_clock_levels_with_voltage *clks, int *valid_clock_levels)
{
- int i;
-
- if (clks->num_levels == 0)
- return false;
+ int i, level = 0;
+ static float valid_clocks_hz[4];
+ if (clks->num_levels == 0) {
+ *valid_clock_levels = level;
+ return valid_clocks_hz;
+ }
- for (i = 0; i < clks->num_levels; i++)
+ for (i = 0; i < clks->num_levels; i++) {
/* Ensure that the result is sane */
- if (clks->data[i].clocks_in_khz == 0)
- return false;
+ if (clks->data[i].clocks_in_khz == 0) {
+ continue;
+ }
+ valid_clocks_hz[level] = clks->data[i].clocks_in_khz / 1000.0;
+ ++level;
+ }
+ *valid_clock_levels = level;
- return true;
+ return valid_clocks_hz;
}
void dcn_bw_update_from_pplib(struct dc *dc)
@@ -1347,28 +1354,34 @@ void dcn_bw_update_from_pplib(struct dc *dc)
struct dc_context *ctx = dc->ctx;
struct dm_pp_clock_levels_with_voltage fclks = {0}, dcfclks = {0};
bool res;
+ int valid_clock_levels = 0;
+ float *valid_clocks_hz;
/* TODO: This is not the proper way to obtain fabric_and_dram_bandwidth, should be min(fclk, memclk) */
- res = dm_pp_get_clock_levels_by_type_with_voltage(
- ctx, DM_PP_CLOCK_TYPE_FCLK, &fclks);
-
+ res = dm_pp_get_clock_levels_by_type_with_voltage(ctx, DM_PP_CLOCK_TYPE_FCLK, &fclks);
kernel_fpu_begin();
- if (res)
- res = verify_clock_values(&fclks);
-
if (res) {
- ASSERT(fclks.num_levels >= 3);
- dc->dcn_soc->fabric_and_dram_bandwidth_vmin0p65 = 32 * (fclks.data[0].clocks_in_khz / 1000.0) / 1000.0;
+ valid_clocks_hz = verify_clock_values(&fclks, &valid_clock_levels);
+ }
+
+ DRM_DEBUG_DRIVER(
+ "DCN_CALC_CS: Reading FCLK values %s - number of valid clock levels: %d\n",
+ (res != false ? "was successful" : "failed"),
+ valid_clock_levels
+ );
+
+ if (res && valid_clock_levels >= 3) {
+ dc->dcn_soc->fabric_and_dram_bandwidth_vmin0p65 = 32 * valid_clocks_hz[0] / 1000.0;
dc->dcn_soc->fabric_and_dram_bandwidth_vmid0p72 = dc->dcn_soc->number_of_channels *
- (fclks.data[fclks.num_levels - (fclks.num_levels > 2 ? 3 : 2)].clocks_in_khz / 1000.0)
- * ddr4_dram_factor_single_Channel / 1000.0;
+ (valid_clocks_hz[valid_clock_levels - 3]) *
+ ddr4_dram_factor_single_Channel / 1000.0;
dc->dcn_soc->fabric_and_dram_bandwidth_vnom0p8 = dc->dcn_soc->number_of_channels *
- (fclks.data[fclks.num_levels - 2].clocks_in_khz / 1000.0)
- * ddr4_dram_factor_single_Channel / 1000.0;
+ (valid_clocks_hz[valid_clock_levels - 2]) *
+ ddr4_dram_factor_single_Channel / 1000.0;
dc->dcn_soc->fabric_and_dram_bandwidth_vmax0p9 = dc->dcn_soc->number_of_channels *
- (fclks.data[fclks.num_levels - 1].clocks_in_khz / 1000.0)
- * ddr4_dram_factor_single_Channel / 1000.0;
+ (valid_clocks_hz[valid_clock_levels - 1]) *
+ ddr4_dram_factor_single_Channel / 1000.0;
} else
BREAK_TO_DEBUGGER();
@@ -1379,14 +1392,20 @@ void dcn_bw_update_from_pplib(struct dc *dc)
kernel_fpu_begin();
+ DRM_DEBUG_DRIVER(
+ "DCN_CALC_CS: Reading DCFCLK values %s - number of valid clock levels: %d\n",
+ (res != false ? "was successful" : "failed"),
+ valid_clock_levels
+ );
+
if (res)
- res = verify_clock_values(&dcfclks);
+ valid_clocks_hz = verify_clock_values(&dcfclks, &valid_clock_levels);
if (res && dcfclks.num_levels >= 3) {
- dc->dcn_soc->dcfclkv_min0p65 = dcfclks.data[0].clocks_in_khz / 1000.0;
- dc->dcn_soc->dcfclkv_mid0p72 = dcfclks.data[dcfclks.num_levels - 3].clocks_in_khz / 1000.0;
- dc->dcn_soc->dcfclkv_nom0p8 = dcfclks.data[dcfclks.num_levels - 2].clocks_in_khz / 1000.0;
- dc->dcn_soc->dcfclkv_max0p9 = dcfclks.data[dcfclks.num_levels - 1].clocks_in_khz / 1000.0;
+ dc->dcn_soc->dcfclkv_min0p65 = valid_clocks_hz[0];
+ dc->dcn_soc->dcfclkv_mid0p72 = valid_clocks_hz[valid_clock_levels - 3];
+ dc->dcn_soc->dcfclkv_nom0p8 = valid_clocks_hz[valid_clock_levels - 2];
+ dc->dcn_soc->dcfclkv_max0p9 = valid_clocks_hz[valid_clock_levels - 1];
} else
BREAK_TO_DEBUGGER();
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
