diff mbox series

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

Message ID 20210331122502.1031073-5-Rodrigo.Siqueira@amd.com (mailing list archive)
State New, archived
Headers show
Series drm/amd/display: Base changes for isolating FPU operation in a single place | expand

Commit Message

Rodrigo Siqueira Jordao March 31, 2021, 12:25 p.m. UTC
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.

Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Acked-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 34 ++++++++++++++++---
 .../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, 49 insertions(+), 5 deletions(-)

Comments

Christian König March 31, 2021, 12:47 p.m. UTC | #1
Am 31.03.21 um 14:25 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.
>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Acked-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 34 ++++++++++++++++---
>   .../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, 49 insertions(+), 5 deletions(-)
>
> 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 5dcefe193523..0d95f680b62b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> @@ -40,6 +40,25 @@
>    */
>   
>   static DEFINE_PER_CPU(atomic_t, fpu_ref);
> +static DEFINE_PER_CPU(atomic_t, fp_dc_enabled);
> +
> +/**
> + * is_fp_dc_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 is_fp_dc_enabled(void)

I would rather name this dc_is_fp_enabled() or even better directly make 
this dc_assert_fp_enabled().

> +{
> +	atomic_t *fp_enabled = this_cpu_ptr(&fp_dc_enabled);
> +
> +	return atomic_read(fp_enabled);

The handling with fp_enaled is overkill. Instead you can also check 
fpu_ref for > 1.

Regards,
Christian.

> +}
>   
>   /**
>    * dc_fpu_begin - Enables FPU protection
> @@ -55,12 +74,15 @@ void dc_fpu_begin(const char *function_name, const int line)
>   {
>   	int ret;
>   	atomic_t *local_fpu_ref = this_cpu_ptr(&fpu_ref);
> +	atomic_t *fp_enabled = this_cpu_ptr(&fp_dc_enabled);
>   
>   	ret = atomic_inc_return(local_fpu_ref);
>   	TRACE_DCN_FPU(true, function_name, line, ret);
>   
> -	if (ret == 1)
> +	if (ret == 1) {
>   		kernel_fpu_begin();
> +		atomic_set(fp_enabled, 1);
> +	}
>   }
>   
>   /**
> @@ -75,13 +97,15 @@ void dc_fpu_begin(const char *function_name, const int line)
>    */
>   void dc_fpu_end(const char *function_name, const int line)
>   {
> -
> -	int ret;
> +	bool ret;
>   	atomic_t *local_fpu_ref = this_cpu_ptr(&fpu_ref);
> +	atomic_t *fp_enabled = this_cpu_ptr(&fp_dc_enabled);
>   
> -	ret = atomic_dec_return(local_fpu_ref);
> +	ret = atomic_dec_and_test(local_fpu_ref);
>   	TRACE_DCN_FPU(false, function_name, line, ret);
>   
> -	if (!ret)
> +	if (ret) {
> +		atomic_set(fp_enabled, 0);
>   		kernel_fpu_end();
> +	}
>   }
> 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..e782dfa640bf 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 is_fp_dc_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 b58edd012038..d0771e29c243 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> @@ -2351,7 +2351,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 32b23a182428..1c8a97d342c0 100644
> --- a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
> +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
> @@ -42,6 +42,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
> + *    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(is_fp_dc_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,
> @@ -84,6 +100,7 @@ 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(is_fp_dc_enabled());
>   	_dcn20_populate_dml_writeback_from_context(dc, res_ctx, pipes);
>   }
>
diff mbox series

Patch

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 5dcefe193523..0d95f680b62b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
@@ -40,6 +40,25 @@ 
  */
 
 static DEFINE_PER_CPU(atomic_t, fpu_ref);
+static DEFINE_PER_CPU(atomic_t, fp_dc_enabled);
+
+/**
+ * is_fp_dc_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 is_fp_dc_enabled(void)
+{
+	atomic_t *fp_enabled = this_cpu_ptr(&fp_dc_enabled);
+
+	return atomic_read(fp_enabled);
+}
 
 /**
  * dc_fpu_begin - Enables FPU protection
@@ -55,12 +74,15 @@  void dc_fpu_begin(const char *function_name, const int line)
 {
 	int ret;
 	atomic_t *local_fpu_ref = this_cpu_ptr(&fpu_ref);
+	atomic_t *fp_enabled = this_cpu_ptr(&fp_dc_enabled);
 
 	ret = atomic_inc_return(local_fpu_ref);
 	TRACE_DCN_FPU(true, function_name, line, ret);
 
-	if (ret == 1)
+	if (ret == 1) {
 		kernel_fpu_begin();
+		atomic_set(fp_enabled, 1);
+	}
 }
 
 /**
@@ -75,13 +97,15 @@  void dc_fpu_begin(const char *function_name, const int line)
  */
 void dc_fpu_end(const char *function_name, const int line)
 {
-
-	int ret;
+	bool ret;
 	atomic_t *local_fpu_ref = this_cpu_ptr(&fpu_ref);
+	atomic_t *fp_enabled = this_cpu_ptr(&fp_dc_enabled);
 
-	ret = atomic_dec_return(local_fpu_ref);
+	ret = atomic_dec_and_test(local_fpu_ref);
 	TRACE_DCN_FPU(false, function_name, line, ret);
 
-	if (!ret)
+	if (ret) {
+		atomic_set(fp_enabled, 0);
 		kernel_fpu_end();
+	}
 }
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..e782dfa640bf 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 is_fp_dc_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 b58edd012038..d0771e29c243 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
@@ -2351,7 +2351,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 32b23a182428..1c8a97d342c0 100644
--- a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
+++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
@@ -42,6 +42,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
+ *    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(is_fp_dc_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,
@@ -84,6 +100,7 @@  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(is_fp_dc_enabled());
 	_dcn20_populate_dml_writeback_from_context(dc, res_ctx, pipes);
 }