Messages in this thread | | | Subject | Re: [PATCH] amdgpu: Avoid sleeping during FPU critical sections | From | Christian König <> | Date | Tue, 12 Jan 2021 11:58:53 +0100 |
| |
Am 11.01.21 um 17:32 schrieb Daniel Vetter: > On Mon, Jan 11, 2021 at 09:53:56AM +0100, Christian König wrote: >> Am 08.01.21 um 22:58 schrieb Jeremy Cline: >>> dcn20_resource_construct() includes a number of kzalloc(GFP_KERNEL) >>> calls which can sleep, but kernel_fpu_begin() disables preemption and >>> sleeping in this context is invalid. >>> >>> The only places the FPU appears to be required is in the >>> init_soc_bounding_box() function and when calculating the >>> {min,max}_fill_clk_mhz. Narrow the scope to just these two parts to >>> avoid sleeping while using the FPU. >>> >>> Fixes: 7a8a3430be15 ("amdgpu: Wrap FPU dependent functions in dc20") >>> Cc: Timothy Pearson <tpearson@raptorengineering.com> >>> Signed-off-by: Jeremy Cline <jcline@redhat.com> >> Good catch, but I would rather replace the kzalloc(GFP_KERNEL) with a >> kzalloc(GFP_ATOMIC) for now. >> >> We have tons of problems with this DC_FP_START()/DC_FP_END() annotations and >> are even in the process of moving them out of the file because the compiles >> tend to clutter FP registers even outside of the annotated ranges on some >> architectures. > Out of curiosity, what's the plan? Soft-fp implementation for DC so you > can keep the algorithms all unchanged, or redoing them as some fixed point > with careful review and retesting everything? Something else?
My last status is that we want to move all FP related functions into a separate C file.
Then compile only that file with FP enabled and all callers of those functions must protected the function calls with FP enable/disable.
That's the only way we can guarantee that no FP state is corrupted.
Regards, Christian.
> -Daniel > >> Regards, >> Christian. >> >>> --- >>> drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c >>> index e04ecf0fc0db..a4fa5bf016c1 100644 >>> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c >>> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c >>> @@ -3622,6 +3622,7 @@ static bool init_soc_bounding_box(struct dc *dc, >>> if (bb && ASICREV_IS_NAVI12_P(dc->ctx->asic_id.hw_internal_rev)) { >>> int i; >>> + DC_FP_START(); >>> dcn2_0_nv12_soc.sr_exit_time_us = >>> fixed16_to_double_to_cpu(bb->sr_exit_time_us); >>> dcn2_0_nv12_soc.sr_enter_plus_exit_time_us = >>> @@ -3721,6 +3722,7 @@ static bool init_soc_bounding_box(struct dc *dc, >>> dcn2_0_nv12_soc.clock_limits[i].dram_speed_mts = >>> fixed16_to_double_to_cpu(bb->clock_limits[i].dram_speed_mts); >>> } >>> + DC_FP_END(); >>> } >>> if (pool->base.pp_smu) { >>> @@ -3777,8 +3779,6 @@ static bool dcn20_resource_construct( >>> enum dml_project dml_project_version = >>> get_dml_project_version(ctx->asic_id.hw_internal_rev); >>> - DC_FP_START(); >>> - >>> ctx->dc_bios->regs = &bios_regs; >>> pool->base.funcs = &dcn20_res_pool_funcs; >>> @@ -3959,8 +3959,10 @@ static bool dcn20_resource_construct( >>> ranges.reader_wm_sets[i].wm_inst = i; >>> ranges.reader_wm_sets[i].min_drain_clk_mhz = PP_SMU_WM_SET_RANGE_CLK_UNCONSTRAINED_MIN; >>> ranges.reader_wm_sets[i].max_drain_clk_mhz = PP_SMU_WM_SET_RANGE_CLK_UNCONSTRAINED_MAX; >>> + DC_FP_START(); >>> ranges.reader_wm_sets[i].min_fill_clk_mhz = (i > 0) ? (loaded_bb->clock_limits[i - 1].dram_speed_mts / 16) + 1 : 0; >>> ranges.reader_wm_sets[i].max_fill_clk_mhz = loaded_bb->clock_limits[i].dram_speed_mts / 16; >>> + DC_FP_END(); >>> ranges.num_reader_wm_sets = i + 1; >>> } >>> @@ -4125,12 +4127,10 @@ static bool dcn20_resource_construct( >>> pool->base.oem_device = NULL; >>> } >>> - DC_FP_END(); >>> return true; >>> create_fail: >>> - DC_FP_END(); >>> dcn20_resource_destruct(pool); >>> return false;
| |