Re: [PATCH v2 4/4] drm/amd/display: Add DC_FP helper to check FPU state

Am 13.07.21 um 16:06 schrieb Rodrigo Siqueira:

To fully isolate FPU operations in a single place, we must avoid situations where compilers spill FP values to registers due to FP enable in a specific C file. Note that even if we isolate all FPU functions in a single file and call its interface from other files, the compiler might enable the use of FPU before we call DC_FP_START. Nevertheless, it is the programmer's responsibility to invoke DC_FP_START/END in the correct place. To highlight situations where developers forgot to use the FP protection before calling the DC FPU interface functions, we introduce a helper that checks if the function is invoked under FP protection. If not, it will trigger a kernel warning.

Changes since V1:

• Remove fp_enable variables
• Rename dc_is_fp_enabled to dc_assert_fp_enabled
• Replace wrong variable type

Signed-off-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com

.../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c | 22 +++++++++++++++++++ .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h | 1 + .../drm/amd/display/dc/dcn20/dcn20_resource.c | 2 ++ .../drm/amd/display/dc/fpu_operations/dcn2x.c | 17 ++++++++++++++ 4 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c index 73179e9e859a..74153a2816f9 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c @@ -41,6 +41,28 @@

static DEFINE_PER_CPU(int, fpu_recursion_depth);

+/**

• • dc_assert_fp_enabled - Check if FPU protection is enabled
• • This function tells if the code is already under FPU protection or not. A
• • function that works as an API for a set of FPU operations can use this
• • function for checking if the caller invoked it after DC_FP_START(). For
• • example, take a look at dcn2x.c file.
• • Return:
• • Return true if we already enabled FPU protection, otherwise return false.
• */

+inline bool dc_assert_fp_enabled(void)

Assert indicates that you print a warning if the condition isn't meet, but you only return the condition.

Either rename the function or raise the warning directly.

+{

• int *pcpu, depth = 0;
• pcpu = get_cpu_ptr(&fpu_recursion_depth);
• depth = this_cpu_read(fpu_recursion_depth);
• put_cpu_ptr(&fpu_recursion_depth);

Again this doesn't make sense.

Either you use this_cpu_read() or your use get_cpu_ptr()/put_cpu_ptr(), but not both.

• return depth > 1;

+}

• /**
  • dc_fpu_begin - Enables FPU protection
  • @function_name: A string containing the function name for debug purposes

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h index fb54983c5c60..97941794b77c 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h @@ -27,6 +27,7 @@ #ifndef __DC_FPU_H__ #define __DC_FPU_H__

+bool dc_assert_fp_enabled(void); void dc_fpu_begin(const char *function_name, const int line); void dc_fpu_end(const char *function_name, const int line);

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 f99b09643a52..d0b34c7f99dc 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c @@ -2355,7 +2355,9 @@ int dcn20_populate_dml_pipes_from_context( }

/* populate writeback information */

• DC_FP_START(); dc->res_pool->funcs->populate_dml_writeback_from_context(dc, res_ctx, pipes);

• DC_FP_END();

  return pipe_cnt; }

diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c index c815d6c01d64..d8183da0c2b0 100644 --- a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c @@ -41,6 +41,22 @@

• that deals with FP register is contained within this call.
• 3. All function that needs to be accessed outside this file requires a
• public interface that not uses any FPU reference.

• • 4. Developers should not use DC_FP_START/END in this file, but they need to

This needs to be harder, e.g. "Developers must not use....".

Regards, Christian.

• • ensure that the caller invokes it before access any function available in
• • this file. For this reason, public API in this file must invoke
• • ASSERT(dc_assert_fp_enabled());
• • Let's expand a little bit more the idea in the code pattern number for. To
• • fully isolate FPU operations in a single place, we must avoid situations
• • where compilers spill FP values to registers due to FP enable in a specific
• • C file. Note that even if we isolate all FPU functions in a single file and
• • call its interface from other files, the compiler might enable the use of
• • FPU before we call DC_FP_START. Nevertheless, it is the programmer's
• • responsibility to invoke DC_FP_START/END in the correct place. To highlight
• • situations where developers forgot to use the FP protection before calling
• • the DC FPU interface functions, we introduce a helper that checks if the
• • function is invoked under FP protection. If not, it will trigger a kernel
• • warning.

  */

  static noinline void _dcn20_populate_dml_writeback_from_context(struct dc *dc,

@@ -83,5 +99,6 @@ static noinline void _dcn20_populate_dml_writeback_from_context(struct dc *dc, void dcn20_populate_dml_writeback_from_context(struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes) {

• ASSERT(dc_assert_fp_enabled()); _dcn20_populate_dml_writeback_from_context(dc, res_ctx, pipes); }