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@xxxxxxx>
---
.../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);
}
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
