| Message ID | 20210720004914.3060879-5-Rodrigo.Siqueira@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Base changes for isolating FPU operation in a single place | expand |
Am 20.07.21 um 02:49 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 cince V2 (Christian): > - Do not use this_cpu_* between get/put_cpu_ptr(). > - In the kernel documentation, better describe restrictions. > - Make dc_assert_fp_enabled trigger the ASSERT message. > > Changes since V1: > - Remove fp_enable variables > - Rename dc_is_fp_enabled to dc_assert_fp_enabled > - Replace wrong variable type > > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Anson Jacob <Anson.Jacob@amd.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Hersen Wu <hersenxs.wu@amd.com> > Cc: Aric Cyr <aric.cyr@amd.com> > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c | 19 +++++++++++++++++++ > .../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 | 18 ++++++++++++++++++ > 4 files changed, 40 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 d0d3e8a34db5..107e1c50576e 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,25 @@ > > 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. > + */ > +inline void dc_assert_fp_enabled(void) > +{ > + int *pcpu, depth = 0; > + > + pcpu = get_cpu_ptr(&fpu_recursion_depth); > + depth = *pcpu; > + put_cpu_ptr(&fpu_recursion_depth); > + > + ASSERT(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..b8275b397920 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__ > > +void 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 95a0b89302a9..3ea90090264c 100644 > --- a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c > +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c > @@ -43,6 +43,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 **must 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 functions in this file must invoke > + * dc_assert_fp_enabled(); > + * > + * Let's expand a little bit more the idea in the code pattern. 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. > */ > > void dcn20_populate_dml_writeback_from_context(struct dc *dc, > @@ -51,6 +67,8 @@ void dcn20_populate_dml_writeback_from_context(struct dc *dc, > { > int pipe_cnt, i; > > + dc_assert_fp_enabled(); > + > for (i = 0, pipe_cnt = 0; i < dc->res_pool->pipe_count; i++) { > struct dc_writeback_info *wb_info = &res_ctx->pipe_ctx[i].stream->writeback_info[0]; >
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 d0d3e8a34db5..107e1c50576e 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,25 @@ 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. + */ +inline void dc_assert_fp_enabled(void) +{ + int *pcpu, depth = 0; + + pcpu = get_cpu_ptr(&fpu_recursion_depth); + depth = *pcpu; + put_cpu_ptr(&fpu_recursion_depth); + + ASSERT(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..b8275b397920 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__ +void 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 95a0b89302a9..3ea90090264c 100644 --- a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c @@ -43,6 +43,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 **must 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 functions in this file must invoke + * dc_assert_fp_enabled(); + * + * Let's expand a little bit more the idea in the code pattern. 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. */ void dcn20_populate_dml_writeback_from_context(struct dc *dc, @@ -51,6 +67,8 @@ void dcn20_populate_dml_writeback_from_context(struct dc *dc, { int pipe_cnt, i; + dc_assert_fp_enabled(); + for (i = 0, pipe_cnt = 0; i < dc->res_pool->pipe_count; i++) { struct dc_writeback_info *wb_info = &res_ctx->pipe_ctx[i].stream->writeback_info[0];
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 cince V2 (Christian): - Do not use this_cpu_* between get/put_cpu_ptr(). - In the kernel documentation, better describe restrictions. - Make dc_assert_fp_enabled trigger the ASSERT message. Changes since V1: - Remove fp_enable variables - Rename dc_is_fp_enabled to dc_assert_fp_enabled - Replace wrong variable type Cc: Harry Wentland <harry.wentland@amd.com> Cc: Anson Jacob <Anson.Jacob@amd.com> Cc: Christian König <christian.koenig@amd.com> Cc: Hersen Wu <hersenxs.wu@amd.com> Cc: Aric Cyr <aric.cyr@amd.com> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> --- .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c | 19 +++++++++++++++++++ .../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 | 18 ++++++++++++++++++ 4 files changed, 40 insertions(+)