From: "Christian König" <christian.koenig@amd.com>
To: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
Peter Zijlstra <peterz@infradead.org>,
Daniel Vetter <daniel@ffwll.ch>,
roman.li@amd.com, sunpeng.li@amd.com
Cc: Anson Jacob <Anson.Jacob@amd.com>,
dri-devel@lists.freedesktop.org, Hersen Wu <hersenxs.wu@amd.com>,
amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 4/4] drm/amd/display: Add DC_FP helper to check FPU state
Date: Tue, 20 Jul 2021 10:13:43 +0200 [thread overview]
Message-ID: <f3876ee5-7fd3-7690-aa40-348e57fa369d@amd.com> (raw)
In-Reply-To: <20210720004914.3060879-5-Rodrigo.Siqueira@amd.com>
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];
>
prev parent reply other threads:[~2021-07-20 8:13 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-20 0:49 [PATCH v3 0/4] Base changes for isolating FPU operation in a single place Rodrigo Siqueira
2021-07-20 0:49 ` [PATCH v3 1/4] drm/amd/display: Introduce FPU directory inside DC Rodrigo Siqueira
2021-07-20 8:09 ` Christian König
2021-07-20 0:49 ` [PATCH v3 2/4] drm/amd/display: Add FPU event trace Rodrigo Siqueira
2021-07-20 8:11 ` Christian König
2021-07-21 11:44 ` Rodrigo Siqueira
2021-07-21 11:52 ` Christian König
2021-07-20 0:49 ` [PATCH v3 3/4] drm/amd/display: Add control mechanism for FPU utilization Rodrigo Siqueira
2021-07-20 8:17 ` Christian König
2021-07-20 0:49 ` [PATCH v3 4/4] drm/amd/display: Add DC_FP helper to check FPU state Rodrigo Siqueira
2021-07-20 8:13 ` Christian König [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f3876ee5-7fd3-7690-aa40-348e57fa369d@amd.com \
--to=christian.koenig@amd.com \
--cc=Anson.Jacob@amd.com \
--cc=Rodrigo.Siqueira@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=hersenxs.wu@amd.com \
--cc=peterz@infradead.org \
--cc=roman.li@amd.com \
--cc=sunpeng.li@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).