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

[Christian König]

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 at 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);
>   }