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