[PATCH v3 0/4] Base changes for isolating FPU operation in a single place

[PATCH v3 0/4] Base changes for isolating FPU operation in a single place

[Rodrigo Siqueira]

Hi,

In the display core, we utilize floats and doubles units for calculating
modesetting parameters. One side effect of our approach to use double-precision
is the fact that we spread multiple FPU access across our driver, which means
that we can accidentally clobber user space FPU state.

# Challenges

1. Keep in mind that this FPU code is ingrained in our display driver and
performs several crucial tasks. Additionally, we already have multiple
architectures available in the kernel and a large set of users; in other words,
we prefer to avoid a radical approach that might break our user's system.

2. We share our display code with other OSs; thus, we need to maintain the
interoperability between these two systems.

3. We need a mechanism for identifying which function uses FPU registers;
fortunately, Peter Zijlstra wrote a series a couple of months ago where he
introduced an FPU check for objtool. I used the following command for
identifying the potential FPU usage:

 ./tools/objtool/objtool check -Ffa "drivers/gpu/drm/amd/display/dc/ANY_FILE.o"

4. Since our code heavily relies on FPU and the fact that we spread
kernel_fpu_begin/end across multiple functions, we can have some complex
scenarios that will require code refactoring. However, we want to avoid
complicated changes since this is a formula to introduce regressions; we want
something that allows us to fix it in small, safe, and reliable steps.

5. Unfortunately, for legacy reasons, we have some problems in how we program
our FPU access, which in some weird scenarios can generate situations where we
try to enter in the fpu mode multiple times or exit too early.

# Our approach

For trying to solve this problem, we came up with the following strategy:

1. Keep in mind that we are using kernel_fpu_begin/end spread in various areas
and sometimes across multiple functions. If we try to move some of the
functions to an isolated place, we can generate a situation where we can call
the FPU protection more than once, causing multiple warnings. We can deal with
this problem by adding a thin management layer around the kernel_fpu_begin/end
used inside the display.

2. We will need a trace mechanism for this FPU management inside our display
code.

3. After we get the thin layer that manages FPU, we can start to move each
function that uses FPU to a centralized place. Our DQE runs multiple tests in
different ASICs every week; we can take advantage of this to ensure that our
FPU patches work does not introduce any regression. The idea is to work on a
specific part of the code every week (e.g., week 1: DCN2, week 1: DCN2.1,
etc.).

4. Finally, after we can isolate the FPU operations in a single place, we can
altogether remove the FPU flags from other files and eliminate an unnecessary
code introduced to deal with this problem. We can also remove the thin layer
added in the step 3.

# This series

To maintain the interoperability between multiple OSes, we already have a
define named DC_FP_START/END, which is a straightforward wrapper to
kernel_fpu_begin/end in the Linux side. In this series, I decided to expand the
scope of this DC_FP_* wrapper to trace FPU entrance and exit in the display
code, but I also add a mechanism for managing the entrance and exit of
kernel_fpu_begin/end. You can see the details on how I did that in the last two
patches.

I also isolate a simple function that requires FPU access to demonstrate my
strategy for isolating this FPU access in a single place. If this series gets
accepted, the following steps consist of moving all FPU functions weekly until
we isolate everything in the fpu_operation folder.

Change Since V2:
- Make sure to compile FPU operation only when DCN is enabled
  (officially, we only enable it for x86).
- Check cross-compile with ARM and x86_32. Everything looks fine.
- Avoid call this_cpu_* operations between get/put_cpu_ptr.
- Fix GCC warnings.
- Update documentation.
- Update our assert mechanism.
- Remove unnecessary wrappers.

Changes since V1:
- Use a better name for variables.
- Update documentation.
- Avoid preemption.

* See update details per commit message

Best Regards
Rodrigo Siqueira

Rodrigo Siqueira (4):
  drm/amd/display: Introduce FPU directory inside DC
  drm/amd/display: Add FPU event trace
  drm/amd/display: Add control mechanism for FPU utilization
  drm/amd/display: Add DC_FP helper to check FPU state

 .../gpu/drm/amd/display/amdgpu_dm/Makefile    |   4 +
 .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   |  24 ++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 111 ++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    |  34 ++++++
 drivers/gpu/drm/amd/display/dc/Makefile       |   1 +
 drivers/gpu/drm/amd/display/dc/dc_trace.h     |   3 +
 .../drm/amd/display/dc/dcn20/dcn20_resource.c |  41 +------
 .../drm/amd/display/dc/dcn20/dcn20_resource.h |   2 -
 .../drm/amd/display/dc/dcn21/dcn21_resource.c |   2 +
 .../amd/display/dc/fpu_operations/Makefile    |  58 +++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.c | 102 ++++++++++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.h |  33 ++++++
 drivers/gpu/drm/amd/display/dc/os_types.h     |   6 +-
 13 files changed, 379 insertions(+), 42 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h

-- 
2.25.1

[PATCH v3 0/4] Base changes for isolating FPU operation in a single place

[Rodrigo Siqueira]

Hi,

In the display core, we utilize floats and doubles units for calculating
modesetting parameters. One side effect of our approach to use double-precision
is the fact that we spread multiple FPU access across our driver, which means
that we can accidentally clobber user space FPU state.

# Challenges

1. Keep in mind that this FPU code is ingrained in our display driver and
performs several crucial tasks. Additionally, we already have multiple
architectures available in the kernel and a large set of users; in other words,
we prefer to avoid a radical approach that might break our user's system.

2. We share our display code with other OSs; thus, we need to maintain the
interoperability between these two systems.

3. We need a mechanism for identifying which function uses FPU registers;
fortunately, Peter Zijlstra wrote a series a couple of months ago where he
introduced an FPU check for objtool. I used the following command for
identifying the potential FPU usage:

 ./tools/objtool/objtool check -Ffa "drivers/gpu/drm/amd/display/dc/ANY_FILE.o"

4. Since our code heavily relies on FPU and the fact that we spread
kernel_fpu_begin/end across multiple functions, we can have some complex
scenarios that will require code refactoring. However, we want to avoid
complicated changes since this is a formula to introduce regressions; we want
something that allows us to fix it in small, safe, and reliable steps.

5. Unfortunately, for legacy reasons, we have some problems in how we program
our FPU access, which in some weird scenarios can generate situations where we
try to enter in the fpu mode multiple times or exit too early.

# Our approach

For trying to solve this problem, we came up with the following strategy:

1. Keep in mind that we are using kernel_fpu_begin/end spread in various areas
and sometimes across multiple functions. If we try to move some of the
functions to an isolated place, we can generate a situation where we can call
the FPU protection more than once, causing multiple warnings. We can deal with
this problem by adding a thin management layer around the kernel_fpu_begin/end
used inside the display.

2. We will need a trace mechanism for this FPU management inside our display
code.

3. After we get the thin layer that manages FPU, we can start to move each
function that uses FPU to a centralized place. Our DQE runs multiple tests in
different ASICs every week; we can take advantage of this to ensure that our
FPU patches work does not introduce any regression. The idea is to work on a
specific part of the code every week (e.g., week 1: DCN2, week 1: DCN2.1,
etc.).

4. Finally, after we can isolate the FPU operations in a single place, we can
altogether remove the FPU flags from other files and eliminate an unnecessary
code introduced to deal with this problem. We can also remove the thin layer
added in the step 3.

# This series

To maintain the interoperability between multiple OSes, we already have a
define named DC_FP_START/END, which is a straightforward wrapper to
kernel_fpu_begin/end in the Linux side. In this series, I decided to expand the
scope of this DC_FP_* wrapper to trace FPU entrance and exit in the display
code, but I also add a mechanism for managing the entrance and exit of
kernel_fpu_begin/end. You can see the details on how I did that in the last two
patches.

I also isolate a simple function that requires FPU access to demonstrate my
strategy for isolating this FPU access in a single place. If this series gets
accepted, the following steps consist of moving all FPU functions weekly until
we isolate everything in the fpu_operation folder.

Change Since V2:
- Make sure to compile FPU operation only when DCN is enabled
  (officially, we only enable it for x86).
- Check cross-compile with ARM and x86_32. Everything looks fine.
- Avoid call this_cpu_* operations between get/put_cpu_ptr.
- Fix GCC warnings.
- Update documentation.
- Update our assert mechanism.
- Remove unnecessary wrappers.

Changes since V1:
- Use a better name for variables.
- Update documentation.
- Avoid preemption.

* See update details per commit message

Best Regards
Rodrigo Siqueira

Rodrigo Siqueira (4):
  drm/amd/display: Introduce FPU directory inside DC
  drm/amd/display: Add FPU event trace
  drm/amd/display: Add control mechanism for FPU utilization
  drm/amd/display: Add DC_FP helper to check FPU state

 .../gpu/drm/amd/display/amdgpu_dm/Makefile    |   4 +
 .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   |  24 ++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 111 ++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    |  34 ++++++
 drivers/gpu/drm/amd/display/dc/Makefile       |   1 +
 drivers/gpu/drm/amd/display/dc/dc_trace.h     |   3 +
 .../drm/amd/display/dc/dcn20/dcn20_resource.c |  41 +------
 .../drm/amd/display/dc/dcn20/dcn20_resource.h |   2 -
 .../drm/amd/display/dc/dcn21/dcn21_resource.c |   2 +
 .../amd/display/dc/fpu_operations/Makefile    |  58 +++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.c | 102 ++++++++++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.h |  33 ++++++
 drivers/gpu/drm/amd/display/dc/os_types.h     |   6 +-
 13 files changed, 379 insertions(+), 42 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h

-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 1/4] drm/amd/display: Introduce FPU directory inside DC

[Rodrigo Siqueira]

The display core files rely on FPU, which requires to be compiled with
special flags. Ideally, we don't want these FPU operations spread around
the DC code; nevertheless, it happens in the current source. This commit
introduces a new directory named fpu_operations that intends to
centralize all files that require the FPU compilation flag. As part of
this new component, this patch also moves one of the functions
(dcn20_populate_dml_writeback_from_context) that require FPU access to a
single shared file. Notice that this is the first part of the work, and
it does not fix the FPU issue yet; we still need other patches for
achieving the complete FPU isolation.

Changes since V2:
- Christian: Remove unnecessary wrapper.
- lkp: Add missing prototype.
- Only compile the FPU operations if the DCN option is enabled.

Change since V1:
- Update documentation and rebase.

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>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/display/dc/Makefile       |  1 +
 .../drm/amd/display/dc/dcn20/dcn20_resource.c | 39 +--------
 .../drm/amd/display/dc/dcn20/dcn20_resource.h |  2 -
 .../drm/amd/display/dc/dcn21/dcn21_resource.c |  2 +
 .../amd/display/dc/fpu_operations/Makefile    | 58 +++++++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.c | 84 +++++++++++++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.h | 33 ++++++++
 7 files changed, 180 insertions(+), 39 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h

diff --git a/drivers/gpu/drm/amd/display/dc/Makefile b/drivers/gpu/drm/amd/display/dc/Makefile
index 943fcb164876..0de4baefa91e 100644
--- a/drivers/gpu/drm/amd/display/dc/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/Makefile
@@ -35,6 +35,7 @@ DC_LIBS += dcn301
 DC_LIBS += dcn302
 DC_LIBS += dcn303
 DC_LIBS += dcn31
+DC_LIBS += fpu_operations
 endif
 
 DC_LIBS += dce120
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 1b05a37b674d..f99b09643a52 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
@@ -35,6 +35,8 @@
 #include "include/irq_service_interface.h"
 #include "dcn20/dcn20_resource.h"
 
+#include "fpu_operations/dcn2x.h"
+
 #include "dcn10/dcn10_hubp.h"
 #include "dcn10/dcn10_ipp.h"
 #include "dcn20_hubbub.h"
@@ -1974,43 +1976,6 @@ void dcn20_split_stream_for_mpc(
 	ASSERT(primary_pipe->plane_state);
 }
 
-void dcn20_populate_dml_writeback_from_context(
-		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes)
-{
-	int pipe_cnt, i;
-
-	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];
-
-		if (!res_ctx->pipe_ctx[i].stream)
-			continue;
-
-		/* Set writeback information */
-		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
-		pipes[pipe_cnt].dout.num_active_wb++;
-		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
-		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
-		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
-		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
-		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
-		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
-		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
-		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
-		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
-		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
-		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
-			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
-				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
-			else
-				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
-		} else
-			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;
-
-		pipe_cnt++;
-	}
-
-}
-
 int dcn20_populate_dml_pipes_from_context(
 		struct dc *dc,
 		struct dc_state *context,
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
index c8f3127bbcdf..6ec8ff45f0f7 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
@@ -58,8 +58,6 @@ struct pipe_ctx *dcn20_acquire_idle_pipe_for_layer(
 		struct dc_state *state,
 		const struct resource_pool *pool,
 		struct dc_stream_state *stream);
-void dcn20_populate_dml_writeback_from_context(
-		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
 
 struct stream_encoder *dcn20_stream_encoder_create(
 	enum engine_id eng_id,
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
index f3d98e3ba624..85385144e2c5 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
@@ -35,6 +35,8 @@
 #include "include/irq_service_interface.h"
 #include "dcn20/dcn20_resource.h"
 
+#include "fpu_operations/dcn2x.h"
+
 #include "clk_mgr.h"
 #include "dcn10/dcn10_hubp.h"
 #include "dcn10/dcn10_ipp.h"
diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
new file mode 100644
index 000000000000..7f6f90c3f267
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: MIT
+#
+# Copyright 2021 Advanced Micro Devices, Inc.
+#
+# Permission is hereby granted, free of charge, to any person obtaining a
+# copy of this software and associated documentation files (the "Software"),
+# to deal in the Software without restriction, including without limitation
+# the rights to use, copy, modify, merge, publish, distribute, sublicense,
+# and/or sell copies of the Software, and to permit persons to whom the
+# Software is furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+# THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+# OTHER DEALINGS IN THE SOFTWARE.
+#
+#
+# Makefile for fpu operations component.
+#
+
+FPU_OPERATIONS = dcn2x.o
+
+ifdef CONFIG_X86
+fpu_ccflags := -mhard-float -msse
+endif
+
+ifdef CONFIG_PPC64
+fpu_ccflags := -mhard-float -maltivec
+endif
+
+ifdef CONFIG_CC_IS_GCC
+ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+IS_OLD_GCC = 1
+endif
+endif
+
+ifdef CONFIG_X86
+ifdef IS_OLD_GCC
+# Stack alignment mismatch, proceed with caution.
+# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
+# (8B stack alignment).
+fpu_ccflags := -mpreferred-stack-boundary=4
+else
+fpu_ccflags :=  -msse2
+endif
+endif
+
+CFLAGS_$(AMDDALPATH)/dc/fpu_operations/dcn2x.o += $(fpu_ccflags)
+
+AMD_DAL_FPU_OPERATIONS = $(addprefix $(AMDDALPATH)/dc/fpu_operations/,$(FPU_OPERATIONS))
+
+AMD_DISPLAY_FILES += $(AMD_DAL_FPU_OPERATIONS)
diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
new file mode 100644
index 000000000000..95a0b89302a9
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#include "resource.h"
+
+#include "fpu_operations/dcn2x.h"
+
+/**
+ * DOC: DCN2x FPU manipulation Overview
+ *
+ * The DCN architecture relies on FPU operations, which require special
+ * compilation flags and the use of kernel_fpu_begin/end functions; ideally, we
+ * want to avoid spreading FPU access across multiple files. With this idea in
+ * mind, this file aims to centralize all DCN20 and DCN2.1 (DCN2x) functions
+ * that require FPU access in a single place. Code in this file follows the
+ * following code pattern:
+ *
+ * 1. Functions that use FPU operations should be isolated in static functions.
+ * 2. The FPU functions should have the noinline attribute to ensure anything
+ *    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.
+ */
+
+void dcn20_populate_dml_writeback_from_context(struct dc *dc,
+					       struct resource_context *res_ctx,
+					       display_e2e_pipe_params_st *pipes)
+{
+	int pipe_cnt, i;
+
+	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];
+
+		if (!res_ctx->pipe_ctx[i].stream)
+			continue;
+
+		/* Set writeback information */
+		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
+		pipes[pipe_cnt].dout.num_active_wb++;
+		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
+		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
+		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
+		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
+		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
+		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
+		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
+		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
+		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
+		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
+		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
+			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
+				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
+			else
+				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
+		} else {
+			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;
+		}
+
+		pipe_cnt++;
+	}
+}
diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
new file mode 100644
index 000000000000..c060f909164b
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#ifndef __DCN2X_H__
+#define __DCN2X_H__
+
+void dcn20_populate_dml_writeback_from_context(struct dc *dc,
+	struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
+
+#endif /* __DCN2X_H__ */
-- 
2.25.1

[PATCH v3 1/4] drm/amd/display: Introduce FPU directory inside DC

[Rodrigo Siqueira]

The display core files rely on FPU, which requires to be compiled with
special flags. Ideally, we don't want these FPU operations spread around
the DC code; nevertheless, it happens in the current source. This commit
introduces a new directory named fpu_operations that intends to
centralize all files that require the FPU compilation flag. As part of
this new component, this patch also moves one of the functions
(dcn20_populate_dml_writeback_from_context) that require FPU access to a
single shared file. Notice that this is the first part of the work, and
it does not fix the FPU issue yet; we still need other patches for
achieving the complete FPU isolation.

Changes since V2:
- Christian: Remove unnecessary wrapper.
- lkp: Add missing prototype.
- Only compile the FPU operations if the DCN option is enabled.

Change since V1:
- Update documentation and rebase.

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>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/display/dc/Makefile       |  1 +
 .../drm/amd/display/dc/dcn20/dcn20_resource.c | 39 +--------
 .../drm/amd/display/dc/dcn20/dcn20_resource.h |  2 -
 .../drm/amd/display/dc/dcn21/dcn21_resource.c |  2 +
 .../amd/display/dc/fpu_operations/Makefile    | 58 +++++++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.c | 84 +++++++++++++++++++
 .../drm/amd/display/dc/fpu_operations/dcn2x.h | 33 ++++++++
 7 files changed, 180 insertions(+), 39 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
 create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h

diff --git a/drivers/gpu/drm/amd/display/dc/Makefile b/drivers/gpu/drm/amd/display/dc/Makefile
index 943fcb164876..0de4baefa91e 100644
--- a/drivers/gpu/drm/amd/display/dc/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/Makefile
@@ -35,6 +35,7 @@ DC_LIBS += dcn301
 DC_LIBS += dcn302
 DC_LIBS += dcn303
 DC_LIBS += dcn31
+DC_LIBS += fpu_operations
 endif
 
 DC_LIBS += dce120
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 1b05a37b674d..f99b09643a52 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
@@ -35,6 +35,8 @@
 #include "include/irq_service_interface.h"
 #include "dcn20/dcn20_resource.h"
 
+#include "fpu_operations/dcn2x.h"
+
 #include "dcn10/dcn10_hubp.h"
 #include "dcn10/dcn10_ipp.h"
 #include "dcn20_hubbub.h"
@@ -1974,43 +1976,6 @@ void dcn20_split_stream_for_mpc(
 	ASSERT(primary_pipe->plane_state);
 }
 
-void dcn20_populate_dml_writeback_from_context(
-		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes)
-{
-	int pipe_cnt, i;
-
-	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];
-
-		if (!res_ctx->pipe_ctx[i].stream)
-			continue;
-
-		/* Set writeback information */
-		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
-		pipes[pipe_cnt].dout.num_active_wb++;
-		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
-		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
-		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
-		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
-		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
-		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
-		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
-		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
-		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
-		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
-		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
-			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
-				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
-			else
-				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
-		} else
-			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;
-
-		pipe_cnt++;
-	}
-
-}
-
 int dcn20_populate_dml_pipes_from_context(
 		struct dc *dc,
 		struct dc_state *context,
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
index c8f3127bbcdf..6ec8ff45f0f7 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
@@ -58,8 +58,6 @@ struct pipe_ctx *dcn20_acquire_idle_pipe_for_layer(
 		struct dc_state *state,
 		const struct resource_pool *pool,
 		struct dc_stream_state *stream);
-void dcn20_populate_dml_writeback_from_context(
-		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
 
 struct stream_encoder *dcn20_stream_encoder_create(
 	enum engine_id eng_id,
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
index f3d98e3ba624..85385144e2c5 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
@@ -35,6 +35,8 @@
 #include "include/irq_service_interface.h"
 #include "dcn20/dcn20_resource.h"
 
+#include "fpu_operations/dcn2x.h"
+
 #include "clk_mgr.h"
 #include "dcn10/dcn10_hubp.h"
 #include "dcn10/dcn10_ipp.h"
diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
new file mode 100644
index 000000000000..7f6f90c3f267
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: MIT
+#
+# Copyright 2021 Advanced Micro Devices, Inc.
+#
+# Permission is hereby granted, free of charge, to any person obtaining a
+# copy of this software and associated documentation files (the "Software"),
+# to deal in the Software without restriction, including without limitation
+# the rights to use, copy, modify, merge, publish, distribute, sublicense,
+# and/or sell copies of the Software, and to permit persons to whom the
+# Software is furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+# THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+# OTHER DEALINGS IN THE SOFTWARE.
+#
+#
+# Makefile for fpu operations component.
+#
+
+FPU_OPERATIONS = dcn2x.o
+
+ifdef CONFIG_X86
+fpu_ccflags := -mhard-float -msse
+endif
+
+ifdef CONFIG_PPC64
+fpu_ccflags := -mhard-float -maltivec
+endif
+
+ifdef CONFIG_CC_IS_GCC
+ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+IS_OLD_GCC = 1
+endif
+endif
+
+ifdef CONFIG_X86
+ifdef IS_OLD_GCC
+# Stack alignment mismatch, proceed with caution.
+# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
+# (8B stack alignment).
+fpu_ccflags := -mpreferred-stack-boundary=4
+else
+fpu_ccflags :=  -msse2
+endif
+endif
+
+CFLAGS_$(AMDDALPATH)/dc/fpu_operations/dcn2x.o += $(fpu_ccflags)
+
+AMD_DAL_FPU_OPERATIONS = $(addprefix $(AMDDALPATH)/dc/fpu_operations/,$(FPU_OPERATIONS))
+
+AMD_DISPLAY_FILES += $(AMD_DAL_FPU_OPERATIONS)
diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
new file mode 100644
index 000000000000..95a0b89302a9
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#include "resource.h"
+
+#include "fpu_operations/dcn2x.h"
+
+/**
+ * DOC: DCN2x FPU manipulation Overview
+ *
+ * The DCN architecture relies on FPU operations, which require special
+ * compilation flags and the use of kernel_fpu_begin/end functions; ideally, we
+ * want to avoid spreading FPU access across multiple files. With this idea in
+ * mind, this file aims to centralize all DCN20 and DCN2.1 (DCN2x) functions
+ * that require FPU access in a single place. Code in this file follows the
+ * following code pattern:
+ *
+ * 1. Functions that use FPU operations should be isolated in static functions.
+ * 2. The FPU functions should have the noinline attribute to ensure anything
+ *    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.
+ */
+
+void dcn20_populate_dml_writeback_from_context(struct dc *dc,
+					       struct resource_context *res_ctx,
+					       display_e2e_pipe_params_st *pipes)
+{
+	int pipe_cnt, i;
+
+	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];
+
+		if (!res_ctx->pipe_ctx[i].stream)
+			continue;
+
+		/* Set writeback information */
+		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
+		pipes[pipe_cnt].dout.num_active_wb++;
+		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
+		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
+		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
+		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
+		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
+		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
+		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
+		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
+		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
+		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
+		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
+			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
+				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
+			else
+				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
+		} else {
+			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;
+		}
+
+		pipe_cnt++;
+	}
+}
diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
new file mode 100644
index 000000000000..c060f909164b
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#ifndef __DCN2X_H__
+#define __DCN2X_H__
+
+void dcn20_populate_dml_writeback_from_context(struct dc *dc,
+	struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
+
+#endif /* __DCN2X_H__ */
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Rodrigo Siqueira]

We don't have any mechanism for tracing FPU operations inside the
display core, making the debug work a little bit tricky. This commit
introduces a trace mechanism inside our DC_FP_START/END macros for
trying to alleviate this problem.

Changes since V2:
- Make sure that we compile FPU operation only in the context that DCN
  is enabled.

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/Makefile    |  4 ++
 .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
 drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
 drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
 6 files changed, 128 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
index 91fb72c96545..718e123a3230 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
@@ -27,6 +27,10 @@
 
 AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
 
+ifdef CONFIG_DRM_AMD_DC_DCN
+AMDGPUDM += dc_fpu.o
+endif
+
 ifneq ($(CONFIG_DRM_AMD_DC),)
 AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
 endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
index 46a33f64cf8e..230bb12c405e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
@@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
 		  __entry->refresh_rate_ns)
 );
 
+TRACE_EVENT(dcn_fpu,
+	    TP_PROTO(bool begin, const char *function, const int line),
+	    TP_ARGS(begin, function, line),
+
+	    TP_STRUCT__entry(
+			     __field(bool, begin)
+			     __field(const char *, function)
+			     __field(int, line)
+	    ),
+	    TP_fast_assign(
+			   __entry->begin = begin;
+			   __entry->function = function;
+			   __entry->line = line;
+	    ),
+	    TP_printk("%s()+%d: %s",
+		      __entry->function,
+		      __entry->line,
+		      __entry->begin ? "begin" : "end"
+	    )
+);
+
 #endif /* _AMDGPU_DM_TRACE_H_ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
new file mode 100644
index 000000000000..d5d156a4517e
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#include "dc_trace.h"
+
+#include <asm/fpu/api.h>
+
+/**
+ * dc_fpu_begin - Enables FPU protection
+ * @function_name: A string containing the function name for debug purposes
+ *   (usually __func__)
+ *
+ * @line: A line number where DC_FP_START was invoked for debug purpose
+ *   (usually __LINE__)
+ *
+ * This function is responsible for managing the use of kernel_fpu_begin() with
+ * the advantage of providing an event trace for debugging.
+ *
+ * Note: Do not call this function directly; always use DC_FP_START().
+ */
+void dc_fpu_begin(const char *function_name, const int line)
+{
+	TRACE_DCN_FPU(true, function_name, line);
+	kernel_fpu_begin();
+}
+
+/**
+ * dc_fpu_end - Disable FPU protection
+ * @function_name: A string containing the function name for debug purposes
+ * @line: A-line number where DC_FP_END was invoked for debug purpose
+ *
+ * This function is responsible for managing the use of kernel_fpu_end() with
+ * the advantage of providing an event trace for debugging.
+ *
+ * Note: Do not call this function directly; always use DC_FP_END().
+ */
+void dc_fpu_end(const char *function_name, const int line)
+{
+	TRACE_DCN_FPU(false, function_name, line);
+	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
new file mode 100644
index 000000000000..fb54983c5c60
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#ifndef __DC_FPU_H__
+#define __DC_FPU_H__
+
+void dc_fpu_begin(const char *function_name, const int line);
+void dc_fpu_end(const char *function_name, const int line);
+
+#endif /* __DC_FPU_H__ */
diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
index d2615357269b..d598ba697e45 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
@@ -37,3 +37,6 @@
 
 #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
 	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
+
+#define TRACE_DCN_FPU(begin, function, line) \
+	trace_dcn_fpu(begin, function, line)
diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
index 126c2f3a4dd3..2ba49aef370d 100644
--- a/drivers/gpu/drm/amd/display/dc/os_types.h
+++ b/drivers/gpu/drm/amd/display/dc/os_types.h
@@ -52,9 +52,9 @@
 
 #if defined(CONFIG_DRM_AMD_DC_DCN)
 #if defined(CONFIG_X86)
-#include <asm/fpu/api.h>
-#define DC_FP_START() kernel_fpu_begin()
-#define DC_FP_END() kernel_fpu_end()
+#include "amdgpu_dm/dc_fpu.h"
+#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
+#define DC_FP_END() dc_fpu_end(__func__, __LINE__)
 #elif defined(CONFIG_PPC64)
 #include <asm/switch_to.h>
 #include <asm/cputable.h>
-- 
2.25.1

[PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Rodrigo Siqueira]

We don't have any mechanism for tracing FPU operations inside the
display core, making the debug work a little bit tricky. This commit
introduces a trace mechanism inside our DC_FP_START/END macros for
trying to alleviate this problem.

Changes since V2:
- Make sure that we compile FPU operation only in the context that DCN
  is enabled.

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/Makefile    |  4 ++
 .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
 drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
 drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
 6 files changed, 128 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
index 91fb72c96545..718e123a3230 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
@@ -27,6 +27,10 @@
 
 AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
 
+ifdef CONFIG_DRM_AMD_DC_DCN
+AMDGPUDM += dc_fpu.o
+endif
+
 ifneq ($(CONFIG_DRM_AMD_DC),)
 AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
 endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
index 46a33f64cf8e..230bb12c405e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
@@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
 		  __entry->refresh_rate_ns)
 );
 
+TRACE_EVENT(dcn_fpu,
+	    TP_PROTO(bool begin, const char *function, const int line),
+	    TP_ARGS(begin, function, line),
+
+	    TP_STRUCT__entry(
+			     __field(bool, begin)
+			     __field(const char *, function)
+			     __field(int, line)
+	    ),
+	    TP_fast_assign(
+			   __entry->begin = begin;
+			   __entry->function = function;
+			   __entry->line = line;
+	    ),
+	    TP_printk("%s()+%d: %s",
+		      __entry->function,
+		      __entry->line,
+		      __entry->begin ? "begin" : "end"
+	    )
+);
+
 #endif /* _AMDGPU_DM_TRACE_H_ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
new file mode 100644
index 000000000000..d5d156a4517e
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#include "dc_trace.h"
+
+#include <asm/fpu/api.h>
+
+/**
+ * dc_fpu_begin - Enables FPU protection
+ * @function_name: A string containing the function name for debug purposes
+ *   (usually __func__)
+ *
+ * @line: A line number where DC_FP_START was invoked for debug purpose
+ *   (usually __LINE__)
+ *
+ * This function is responsible for managing the use of kernel_fpu_begin() with
+ * the advantage of providing an event trace for debugging.
+ *
+ * Note: Do not call this function directly; always use DC_FP_START().
+ */
+void dc_fpu_begin(const char *function_name, const int line)
+{
+	TRACE_DCN_FPU(true, function_name, line);
+	kernel_fpu_begin();
+}
+
+/**
+ * dc_fpu_end - Disable FPU protection
+ * @function_name: A string containing the function name for debug purposes
+ * @line: A-line number where DC_FP_END was invoked for debug purpose
+ *
+ * This function is responsible for managing the use of kernel_fpu_end() with
+ * the advantage of providing an event trace for debugging.
+ *
+ * Note: Do not call this function directly; always use DC_FP_END().
+ */
+void dc_fpu_end(const char *function_name, const int line)
+{
+	TRACE_DCN_FPU(false, function_name, line);
+	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
new file mode 100644
index 000000000000..fb54983c5c60
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2021 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#ifndef __DC_FPU_H__
+#define __DC_FPU_H__
+
+void dc_fpu_begin(const char *function_name, const int line);
+void dc_fpu_end(const char *function_name, const int line);
+
+#endif /* __DC_FPU_H__ */
diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
index d2615357269b..d598ba697e45 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
@@ -37,3 +37,6 @@
 
 #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
 	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
+
+#define TRACE_DCN_FPU(begin, function, line) \
+	trace_dcn_fpu(begin, function, line)
diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
index 126c2f3a4dd3..2ba49aef370d 100644
--- a/drivers/gpu/drm/amd/display/dc/os_types.h
+++ b/drivers/gpu/drm/amd/display/dc/os_types.h
@@ -52,9 +52,9 @@
 
 #if defined(CONFIG_DRM_AMD_DC_DCN)
 #if defined(CONFIG_X86)
-#include <asm/fpu/api.h>
-#define DC_FP_START() kernel_fpu_begin()
-#define DC_FP_END() kernel_fpu_end()
+#include "amdgpu_dm/dc_fpu.h"
+#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
+#define DC_FP_END() dc_fpu_end(__func__, __LINE__)
 #elif defined(CONFIG_PPC64)
 #include <asm/switch_to.h>
 #include <asm/cputable.h>
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 3/4] drm/amd/display: Add control mechanism for FPU utilization

[Rodrigo Siqueira]

DC invokes DC_FPU_START/END in multiple parts of the code; this can
create a situation where we invoke this FPU operation in a nested way or
exit too early. For avoiding this situation, this commit adds a
mechanism where dc_fpu_begin/end manages the access to
kernel_fpu_begin/end.

Change since V2:
- Christian: Do not use this_cpu_* between get/put_cpu_ptr().

Change since V1:
- Use a better variable names
- Use get_cpu_ptr and put_cpu_ptr to better balance preemption enable
and disable

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>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 13 ++++---
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 36 ++++++++++++++++---
 drivers/gpu/drm/amd/display/dc/dc_trace.h     |  4 +--
 3 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
index 230bb12c405e..fdcaea22b456 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
@@ -638,23 +638,26 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
 );
 
 TRACE_EVENT(dcn_fpu,
-	    TP_PROTO(bool begin, const char *function, const int line),
-	    TP_ARGS(begin, function, line),
+	    TP_PROTO(bool begin, const char *function, const int line, const int recursion_depth),
+	    TP_ARGS(begin, function, line, recursion_depth),
 
 	    TP_STRUCT__entry(
 			     __field(bool, begin)
 			     __field(const char *, function)
 			     __field(int, line)
+			     __field(int, recursion_depth)
 	    ),
 	    TP_fast_assign(
 			   __entry->begin = begin;
 			   __entry->function = function;
 			   __entry->line = line;
+			   __entry->recursion_depth = recursion_depth;
 	    ),
-	    TP_printk("%s()+%d: %s",
+	    TP_printk("%s: recursion_depth: %d: %s()+%d:",
+		      __entry->begin ? "begin" : "end",
+		      __entry->recursion_depth,
 		      __entry->function,
-		      __entry->line,
-		      __entry->begin ? "begin" : "end"
+		      __entry->line
 	    )
 );
 
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 d5d156a4517e..d0d3e8a34db5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
@@ -28,6 +28,19 @@
 
 #include <asm/fpu/api.h>
 
+/**
+ * DOC: DC FPU manipulation overview
+ *
+ * DC core uses FPU operations in multiple parts of the code, which requires a
+ * more specialized way to manage these areas' entrance. To fulfill this
+ * requirement, we created some wrapper functions that encapsulate
+ * kernel_fpu_begin/end to better fit our need in the display component. In
+ * summary, in this file, you can find functions related to FPU operation
+ * management.
+ */
+
+static DEFINE_PER_CPU(int, fpu_recursion_depth);
+
 /**
  * dc_fpu_begin - Enables FPU protection
  * @function_name: A string containing the function name for debug purposes
@@ -43,8 +56,16 @@
  */
 void dc_fpu_begin(const char *function_name, const int line)
 {
-	TRACE_DCN_FPU(true, function_name, line);
-	kernel_fpu_begin();
+	int *pcpu;
+
+	pcpu = get_cpu_ptr(&fpu_recursion_depth);
+	*pcpu += 1;
+
+	if (*pcpu == 1)
+		kernel_fpu_begin();
+
+	TRACE_DCN_FPU(true, function_name, line, *pcpu);
+	put_cpu_ptr(&fpu_recursion_depth);
 }
 
 /**
@@ -59,6 +80,13 @@ void dc_fpu_begin(const char *function_name, const int line)
  */
 void dc_fpu_end(const char *function_name, const int line)
 {
-	TRACE_DCN_FPU(false, function_name, line);
-	kernel_fpu_end();
+	int *pcpu;
+
+	pcpu = get_cpu_ptr(&fpu_recursion_depth);
+	*pcpu -= 1;
+	if (*pcpu <= 0)
+		kernel_fpu_end();
+
+	TRACE_DCN_FPU(false, function_name, line, *pcpu);
+	put_cpu_ptr(&fpu_recursion_depth);
 }
diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
index d598ba697e45..c711797e5c9e 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
@@ -38,5 +38,5 @@
 #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
 	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
 
-#define TRACE_DCN_FPU(begin, function, line) \
-	trace_dcn_fpu(begin, function, line)
+#define TRACE_DCN_FPU(begin, function, line, ref_count) \
+	trace_dcn_fpu(begin, function, line, ref_count)
-- 
2.25.1

[PATCH v3 3/4] drm/amd/display: Add control mechanism for FPU utilization

[Rodrigo Siqueira]

DC invokes DC_FPU_START/END in multiple parts of the code; this can
create a situation where we invoke this FPU operation in a nested way or
exit too early. For avoiding this situation, this commit adds a
mechanism where dc_fpu_begin/end manages the access to
kernel_fpu_begin/end.

Change since V2:
- Christian: Do not use this_cpu_* between get/put_cpu_ptr().

Change since V1:
- Use a better variable names
- Use get_cpu_ptr and put_cpu_ptr to better balance preemption enable
and disable

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>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 13 ++++---
 .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 36 ++++++++++++++++---
 drivers/gpu/drm/amd/display/dc/dc_trace.h     |  4 +--
 3 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
index 230bb12c405e..fdcaea22b456 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
@@ -638,23 +638,26 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
 );
 
 TRACE_EVENT(dcn_fpu,
-	    TP_PROTO(bool begin, const char *function, const int line),
-	    TP_ARGS(begin, function, line),
+	    TP_PROTO(bool begin, const char *function, const int line, const int recursion_depth),
+	    TP_ARGS(begin, function, line, recursion_depth),
 
 	    TP_STRUCT__entry(
 			     __field(bool, begin)
 			     __field(const char *, function)
 			     __field(int, line)
+			     __field(int, recursion_depth)
 	    ),
 	    TP_fast_assign(
 			   __entry->begin = begin;
 			   __entry->function = function;
 			   __entry->line = line;
+			   __entry->recursion_depth = recursion_depth;
 	    ),
-	    TP_printk("%s()+%d: %s",
+	    TP_printk("%s: recursion_depth: %d: %s()+%d:",
+		      __entry->begin ? "begin" : "end",
+		      __entry->recursion_depth,
 		      __entry->function,
-		      __entry->line,
-		      __entry->begin ? "begin" : "end"
+		      __entry->line
 	    )
 );
 
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 d5d156a4517e..d0d3e8a34db5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
@@ -28,6 +28,19 @@
 
 #include <asm/fpu/api.h>
 
+/**
+ * DOC: DC FPU manipulation overview
+ *
+ * DC core uses FPU operations in multiple parts of the code, which requires a
+ * more specialized way to manage these areas' entrance. To fulfill this
+ * requirement, we created some wrapper functions that encapsulate
+ * kernel_fpu_begin/end to better fit our need in the display component. In
+ * summary, in this file, you can find functions related to FPU operation
+ * management.
+ */
+
+static DEFINE_PER_CPU(int, fpu_recursion_depth);
+
 /**
  * dc_fpu_begin - Enables FPU protection
  * @function_name: A string containing the function name for debug purposes
@@ -43,8 +56,16 @@
  */
 void dc_fpu_begin(const char *function_name, const int line)
 {
-	TRACE_DCN_FPU(true, function_name, line);
-	kernel_fpu_begin();
+	int *pcpu;
+
+	pcpu = get_cpu_ptr(&fpu_recursion_depth);
+	*pcpu += 1;
+
+	if (*pcpu == 1)
+		kernel_fpu_begin();
+
+	TRACE_DCN_FPU(true, function_name, line, *pcpu);
+	put_cpu_ptr(&fpu_recursion_depth);
 }
 
 /**
@@ -59,6 +80,13 @@ void dc_fpu_begin(const char *function_name, const int line)
  */
 void dc_fpu_end(const char *function_name, const int line)
 {
-	TRACE_DCN_FPU(false, function_name, line);
-	kernel_fpu_end();
+	int *pcpu;
+
+	pcpu = get_cpu_ptr(&fpu_recursion_depth);
+	*pcpu -= 1;
+	if (*pcpu <= 0)
+		kernel_fpu_end();
+
+	TRACE_DCN_FPU(false, function_name, line, *pcpu);
+	put_cpu_ptr(&fpu_recursion_depth);
 }
diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
index d598ba697e45..c711797e5c9e 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
@@ -38,5 +38,5 @@
 #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
 	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
 
-#define TRACE_DCN_FPU(begin, function, line) \
-	trace_dcn_fpu(begin, function, line)
+#define TRACE_DCN_FPU(begin, function, line, ref_count) \
+	trace_dcn_fpu(begin, function, line, ref_count)
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[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>
---
 .../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];
 
-- 
2.25.1

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

[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>
---
 .../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];
 
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 1/4] drm/amd/display: Introduce FPU directory inside DC

[Christian König]

Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> The display core files rely on FPU, which requires to be compiled with
> special flags. Ideally, we don't want these FPU operations spread around
> the DC code; nevertheless, it happens in the current source. This commit
> introduces a new directory named fpu_operations that intends to
> centralize all files that require the FPU compilation flag. As part of
> this new component, this patch also moves one of the functions
> (dcn20_populate_dml_writeback_from_context) that require FPU access to a
> single shared file. Notice that this is the first part of the work, and
> it does not fix the FPU issue yet; we still need other patches for
> achieving the complete FPU isolation.
>
> Changes since V2:
> - Christian: Remove unnecessary wrapper.
> - lkp: Add missing prototype.
> - Only compile the FPU operations if the DCN option is enabled.
>
> Change since V1:
> - Update documentation and rebase.
>
> 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>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> ---
>   drivers/gpu/drm/amd/display/dc/Makefile       |  1 +
>   .../drm/amd/display/dc/dcn20/dcn20_resource.c | 39 +--------
>   .../drm/amd/display/dc/dcn20/dcn20_resource.h |  2 -
>   .../drm/amd/display/dc/dcn21/dcn21_resource.c |  2 +
>   .../amd/display/dc/fpu_operations/Makefile    | 58 +++++++++++++
>   .../drm/amd/display/dc/fpu_operations/dcn2x.c | 84 +++++++++++++++++++
>   .../drm/amd/display/dc/fpu_operations/dcn2x.h | 33 ++++++++
>   7 files changed, 180 insertions(+), 39 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
>   create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
>   create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
>
> diff --git a/drivers/gpu/drm/amd/display/dc/Makefile b/drivers/gpu/drm/amd/display/dc/Makefile
> index 943fcb164876..0de4baefa91e 100644
> --- a/drivers/gpu/drm/amd/display/dc/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/Makefile
> @@ -35,6 +35,7 @@ DC_LIBS += dcn301
>   DC_LIBS += dcn302
>   DC_LIBS += dcn303
>   DC_LIBS += dcn31
> +DC_LIBS += fpu_operations
>   endif
>   
>   DC_LIBS += dce120
> 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 1b05a37b674d..f99b09643a52 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> @@ -35,6 +35,8 @@
>   #include "include/irq_service_interface.h"
>   #include "dcn20/dcn20_resource.h"
>   
> +#include "fpu_operations/dcn2x.h"
> +
>   #include "dcn10/dcn10_hubp.h"
>   #include "dcn10/dcn10_ipp.h"
>   #include "dcn20_hubbub.h"
> @@ -1974,43 +1976,6 @@ void dcn20_split_stream_for_mpc(
>   	ASSERT(primary_pipe->plane_state);
>   }
>   
> -void dcn20_populate_dml_writeback_from_context(
> -		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes)

BTW: This is not good coding style.

Maybe use a different editor or re-read the coding style document.

> -{
> -	int pipe_cnt, i;
> -
> -	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];
> -
> -		if (!res_ctx->pipe_ctx[i].stream)
> -			continue;
> -


> -		/* Set writeback information */
> -		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
> -		pipes[pipe_cnt].dout.num_active_wb++;
> -		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
> -		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
> -		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
> -		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
> -		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
> -		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
> -		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
> -		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
> -		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
> -		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
> -		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
> -			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
> -				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
> -			else
> -				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
> -		} else
> -			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;

Ideally you would only have this chunk in the FPU specific directory and 
not the outer loop around it.

But it's at least a start, feel free to add an Reviewed-by: Christian 
König <christian.koenig@amd.com> to the patch and commit it.

Thanks,
Christian.

> -
> -		pipe_cnt++;
> -	}
> -
> -}
> -
>   int dcn20_populate_dml_pipes_from_context(
>   		struct dc *dc,
>   		struct dc_state *context,
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
> index c8f3127bbcdf..6ec8ff45f0f7 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
> @@ -58,8 +58,6 @@ struct pipe_ctx *dcn20_acquire_idle_pipe_for_layer(
>   		struct dc_state *state,
>   		const struct resource_pool *pool,
>   		struct dc_stream_state *stream);
> -void dcn20_populate_dml_writeback_from_context(
> -		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
>   
>   struct stream_encoder *dcn20_stream_encoder_create(
>   	enum engine_id eng_id,
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
> index f3d98e3ba624..85385144e2c5 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
> @@ -35,6 +35,8 @@
>   #include "include/irq_service_interface.h"
>   #include "dcn20/dcn20_resource.h"
>   
> +#include "fpu_operations/dcn2x.h"
> +
>   #include "clk_mgr.h"
>   #include "dcn10/dcn10_hubp.h"
>   #include "dcn10/dcn10_ipp.h"
> diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
> new file mode 100644
> index 000000000000..7f6f90c3f267
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: MIT
> +#
> +# Copyright 2021 Advanced Micro Devices, Inc.
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a
> +# copy of this software and associated documentation files (the "Software"),
> +# to deal in the Software without restriction, including without limitation
> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice shall be included in
> +# all copies or substantial portions of the Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +# THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> +# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> +# OTHER DEALINGS IN THE SOFTWARE.
> +#
> +#
> +# Makefile for fpu operations component.
> +#
> +
> +FPU_OPERATIONS = dcn2x.o
> +
> +ifdef CONFIG_X86
> +fpu_ccflags := -mhard-float -msse
> +endif
> +
> +ifdef CONFIG_PPC64
> +fpu_ccflags := -mhard-float -maltivec
> +endif
> +
> +ifdef CONFIG_CC_IS_GCC
> +ifeq ($(call cc-ifversion, -lt, 0701, y), y)
> +IS_OLD_GCC = 1
> +endif
> +endif
> +
> +ifdef CONFIG_X86
> +ifdef IS_OLD_GCC
> +# Stack alignment mismatch, proceed with caution.
> +# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
> +# (8B stack alignment).
> +fpu_ccflags := -mpreferred-stack-boundary=4
> +else
> +fpu_ccflags :=  -msse2
> +endif
> +endif
> +
> +CFLAGS_$(AMDDALPATH)/dc/fpu_operations/dcn2x.o += $(fpu_ccflags)
> +
> +AMD_DAL_FPU_OPERATIONS = $(addprefix $(AMDDALPATH)/dc/fpu_operations/,$(FPU_OPERATIONS))
> +
> +AMD_DISPLAY_FILES += $(AMD_DAL_FPU_OPERATIONS)
> diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
> new file mode 100644
> index 000000000000..95a0b89302a9
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#include "resource.h"
> +
> +#include "fpu_operations/dcn2x.h"
> +
> +/**
> + * DOC: DCN2x FPU manipulation Overview
> + *
> + * The DCN architecture relies on FPU operations, which require special
> + * compilation flags and the use of kernel_fpu_begin/end functions; ideally, we
> + * want to avoid spreading FPU access across multiple files. With this idea in
> + * mind, this file aims to centralize all DCN20 and DCN2.1 (DCN2x) functions
> + * that require FPU access in a single place. Code in this file follows the
> + * following code pattern:
> + *
> + * 1. Functions that use FPU operations should be isolated in static functions.
> + * 2. The FPU functions should have the noinline attribute to ensure anything
> + *    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.
> + */
> +
> +void dcn20_populate_dml_writeback_from_context(struct dc *dc,
> +					       struct resource_context *res_ctx,
> +					       display_e2e_pipe_params_st *pipes)
> +{
> +	int pipe_cnt, i;
> +
> +	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];
> +
> +		if (!res_ctx->pipe_ctx[i].stream)
> +			continue;
> +
> +		/* Set writeback information */
> +		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
> +		pipes[pipe_cnt].dout.num_active_wb++;
> +		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
> +		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
> +		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
> +		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
> +		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
> +		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
> +		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
> +		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
> +		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
> +		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
> +		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
> +			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
> +				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
> +			else
> +				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
> +		} else {
> +			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;
> +		}
> +
> +		pipe_cnt++;
> +	}
> +}
> diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
> new file mode 100644
> index 000000000000..c060f909164b
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#ifndef __DCN2X_H__
> +#define __DCN2X_H__
> +
> +void dcn20_populate_dml_writeback_from_context(struct dc *dc,
> +	struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
> +
> +#endif /* __DCN2X_H__ */

Re: [PATCH v3 1/4] drm/amd/display: Introduce FPU directory inside DC

[Christian König]

Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> The display core files rely on FPU, which requires to be compiled with
> special flags. Ideally, we don't want these FPU operations spread around
> the DC code; nevertheless, it happens in the current source. This commit
> introduces a new directory named fpu_operations that intends to
> centralize all files that require the FPU compilation flag. As part of
> this new component, this patch also moves one of the functions
> (dcn20_populate_dml_writeback_from_context) that require FPU access to a
> single shared file. Notice that this is the first part of the work, and
> it does not fix the FPU issue yet; we still need other patches for
> achieving the complete FPU isolation.
>
> Changes since V2:
> - Christian: Remove unnecessary wrapper.
> - lkp: Add missing prototype.
> - Only compile the FPU operations if the DCN option is enabled.
>
> Change since V1:
> - Update documentation and rebase.
>
> 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>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> ---
>   drivers/gpu/drm/amd/display/dc/Makefile       |  1 +
>   .../drm/amd/display/dc/dcn20/dcn20_resource.c | 39 +--------
>   .../drm/amd/display/dc/dcn20/dcn20_resource.h |  2 -
>   .../drm/amd/display/dc/dcn21/dcn21_resource.c |  2 +
>   .../amd/display/dc/fpu_operations/Makefile    | 58 +++++++++++++
>   .../drm/amd/display/dc/fpu_operations/dcn2x.c | 84 +++++++++++++++++++
>   .../drm/amd/display/dc/fpu_operations/dcn2x.h | 33 ++++++++
>   7 files changed, 180 insertions(+), 39 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
>   create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
>   create mode 100644 drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
>
> diff --git a/drivers/gpu/drm/amd/display/dc/Makefile b/drivers/gpu/drm/amd/display/dc/Makefile
> index 943fcb164876..0de4baefa91e 100644
> --- a/drivers/gpu/drm/amd/display/dc/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/Makefile
> @@ -35,6 +35,7 @@ DC_LIBS += dcn301
>   DC_LIBS += dcn302
>   DC_LIBS += dcn303
>   DC_LIBS += dcn31
> +DC_LIBS += fpu_operations
>   endif
>   
>   DC_LIBS += dce120
> 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 1b05a37b674d..f99b09643a52 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
> @@ -35,6 +35,8 @@
>   #include "include/irq_service_interface.h"
>   #include "dcn20/dcn20_resource.h"
>   
> +#include "fpu_operations/dcn2x.h"
> +
>   #include "dcn10/dcn10_hubp.h"
>   #include "dcn10/dcn10_ipp.h"
>   #include "dcn20_hubbub.h"
> @@ -1974,43 +1976,6 @@ void dcn20_split_stream_for_mpc(
>   	ASSERT(primary_pipe->plane_state);
>   }
>   
> -void dcn20_populate_dml_writeback_from_context(
> -		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes)

BTW: This is not good coding style.

Maybe use a different editor or re-read the coding style document.

> -{
> -	int pipe_cnt, i;
> -
> -	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];
> -
> -		if (!res_ctx->pipe_ctx[i].stream)
> -			continue;
> -


> -		/* Set writeback information */
> -		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
> -		pipes[pipe_cnt].dout.num_active_wb++;
> -		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
> -		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
> -		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
> -		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
> -		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
> -		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
> -		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
> -		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
> -		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
> -		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
> -		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
> -			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
> -				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
> -			else
> -				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
> -		} else
> -			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;

Ideally you would only have this chunk in the FPU specific directory and 
not the outer loop around it.

But it's at least a start, feel free to add an Reviewed-by: Christian 
König <christian.koenig@amd.com> to the patch and commit it.

Thanks,
Christian.

> -
> -		pipe_cnt++;
> -	}
> -
> -}
> -
>   int dcn20_populate_dml_pipes_from_context(
>   		struct dc *dc,
>   		struct dc_state *context,
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
> index c8f3127bbcdf..6ec8ff45f0f7 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.h
> @@ -58,8 +58,6 @@ struct pipe_ctx *dcn20_acquire_idle_pipe_for_layer(
>   		struct dc_state *state,
>   		const struct resource_pool *pool,
>   		struct dc_stream_state *stream);
> -void dcn20_populate_dml_writeback_from_context(
> -		struct dc *dc, struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
>   
>   struct stream_encoder *dcn20_stream_encoder_create(
>   	enum engine_id eng_id,
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
> index f3d98e3ba624..85385144e2c5 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
> @@ -35,6 +35,8 @@
>   #include "include/irq_service_interface.h"
>   #include "dcn20/dcn20_resource.h"
>   
> +#include "fpu_operations/dcn2x.h"
> +
>   #include "clk_mgr.h"
>   #include "dcn10/dcn10_hubp.h"
>   #include "dcn10/dcn10_ipp.h"
> diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
> new file mode 100644
> index 000000000000..7f6f90c3f267
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/Makefile
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: MIT
> +#
> +# Copyright 2021 Advanced Micro Devices, Inc.
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a
> +# copy of this software and associated documentation files (the "Software"),
> +# to deal in the Software without restriction, including without limitation
> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice shall be included in
> +# all copies or substantial portions of the Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +# THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> +# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> +# OTHER DEALINGS IN THE SOFTWARE.
> +#
> +#
> +# Makefile for fpu operations component.
> +#
> +
> +FPU_OPERATIONS = dcn2x.o
> +
> +ifdef CONFIG_X86
> +fpu_ccflags := -mhard-float -msse
> +endif
> +
> +ifdef CONFIG_PPC64
> +fpu_ccflags := -mhard-float -maltivec
> +endif
> +
> +ifdef CONFIG_CC_IS_GCC
> +ifeq ($(call cc-ifversion, -lt, 0701, y), y)
> +IS_OLD_GCC = 1
> +endif
> +endif
> +
> +ifdef CONFIG_X86
> +ifdef IS_OLD_GCC
> +# Stack alignment mismatch, proceed with caution.
> +# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
> +# (8B stack alignment).
> +fpu_ccflags := -mpreferred-stack-boundary=4
> +else
> +fpu_ccflags :=  -msse2
> +endif
> +endif
> +
> +CFLAGS_$(AMDDALPATH)/dc/fpu_operations/dcn2x.o += $(fpu_ccflags)
> +
> +AMD_DAL_FPU_OPERATIONS = $(addprefix $(AMDDALPATH)/dc/fpu_operations/,$(FPU_OPERATIONS))
> +
> +AMD_DISPLAY_FILES += $(AMD_DAL_FPU_OPERATIONS)
> diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
> new file mode 100644
> index 000000000000..95a0b89302a9
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#include "resource.h"
> +
> +#include "fpu_operations/dcn2x.h"
> +
> +/**
> + * DOC: DCN2x FPU manipulation Overview
> + *
> + * The DCN architecture relies on FPU operations, which require special
> + * compilation flags and the use of kernel_fpu_begin/end functions; ideally, we
> + * want to avoid spreading FPU access across multiple files. With this idea in
> + * mind, this file aims to centralize all DCN20 and DCN2.1 (DCN2x) functions
> + * that require FPU access in a single place. Code in this file follows the
> + * following code pattern:
> + *
> + * 1. Functions that use FPU operations should be isolated in static functions.
> + * 2. The FPU functions should have the noinline attribute to ensure anything
> + *    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.
> + */
> +
> +void dcn20_populate_dml_writeback_from_context(struct dc *dc,
> +					       struct resource_context *res_ctx,
> +					       display_e2e_pipe_params_st *pipes)
> +{
> +	int pipe_cnt, i;
> +
> +	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];
> +
> +		if (!res_ctx->pipe_ctx[i].stream)
> +			continue;
> +
> +		/* Set writeback information */
> +		pipes[pipe_cnt].dout.wb_enable = (wb_info->wb_enabled == true) ? 1 : 0;
> +		pipes[pipe_cnt].dout.num_active_wb++;
> +		pipes[pipe_cnt].dout.wb.wb_src_height = wb_info->dwb_params.cnv_params.crop_height;
> +		pipes[pipe_cnt].dout.wb.wb_src_width = wb_info->dwb_params.cnv_params.crop_width;
> +		pipes[pipe_cnt].dout.wb.wb_dst_width = wb_info->dwb_params.dest_width;
> +		pipes[pipe_cnt].dout.wb.wb_dst_height = wb_info->dwb_params.dest_height;
> +		pipes[pipe_cnt].dout.wb.wb_htaps_luma = 1;
> +		pipes[pipe_cnt].dout.wb.wb_vtaps_luma = 1;
> +		pipes[pipe_cnt].dout.wb.wb_htaps_chroma = wb_info->dwb_params.scaler_taps.h_taps_c;
> +		pipes[pipe_cnt].dout.wb.wb_vtaps_chroma = wb_info->dwb_params.scaler_taps.v_taps_c;
> +		pipes[pipe_cnt].dout.wb.wb_hratio = 1.0;
> +		pipes[pipe_cnt].dout.wb.wb_vratio = 1.0;
> +		if (wb_info->dwb_params.out_format == dwb_scaler_mode_yuv420) {
> +			if (wb_info->dwb_params.output_depth == DWB_OUTPUT_PIXEL_DEPTH_8BPC)
> +				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_8;
> +			else
> +				pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_420_10;
> +		} else {
> +			pipes[pipe_cnt].dout.wb.wb_pixel_format = dm_444_32;
> +		}
> +
> +		pipe_cnt++;
> +	}
> +}
> diff --git a/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
> new file mode 100644
> index 000000000000..c060f909164b
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/fpu_operations/dcn2x.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#ifndef __DCN2X_H__
> +#define __DCN2X_H__
> +
> +void dcn20_populate_dml_writeback_from_context(struct dc *dc,
> +	struct resource_context *res_ctx, display_e2e_pipe_params_st *pipes);
> +
> +#endif /* __DCN2X_H__ */

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Christian König]

Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> We don't have any mechanism for tracing FPU operations inside the
> display core, making the debug work a little bit tricky. This commit
> introduces a trace mechanism inside our DC_FP_START/END macros for
> trying to alleviate this problem.
>
> Changes since V2:
> - Make sure that we compile FPU operation only in the context that DCN
>    is enabled.
>
> 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/Makefile    |  4 ++
>   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
>   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
>   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
>   drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
>   drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
>   6 files changed, 128 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> index 91fb72c96545..718e123a3230 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> @@ -27,6 +27,10 @@
>   
>   AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
>   
> +ifdef CONFIG_DRM_AMD_DC_DCN
> +AMDGPUDM += dc_fpu.o
> +endif
> +
>   ifneq ($(CONFIG_DRM_AMD_DC),)
>   AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
>   endif
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> index 46a33f64cf8e..230bb12c405e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> @@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
>   		  __entry->refresh_rate_ns)
>   );
>   
> +TRACE_EVENT(dcn_fpu,
> +	    TP_PROTO(bool begin, const char *function, const int line),
> +	    TP_ARGS(begin, function, line),
> +
> +	    TP_STRUCT__entry(
> +			     __field(bool, begin)
> +			     __field(const char *, function)
> +			     __field(int, line)
> +	    ),
> +	    TP_fast_assign(
> +			   __entry->begin = begin;
> +			   __entry->function = function;
> +			   __entry->line = line;
> +	    ),
> +	    TP_printk("%s()+%d: %s",
> +		      __entry->function,
> +		      __entry->line,
> +		      __entry->begin ? "begin" : "end"
> +	    )
> +);
> +
>   #endif /* _AMDGPU_DM_TRACE_H_ */
>   
>   #undef TRACE_INCLUDE_PATH
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> new file mode 100644
> index 000000000000..d5d156a4517e
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#include "dc_trace.h"
> +
> +#include <asm/fpu/api.h>
> +
> +/**
> + * dc_fpu_begin - Enables FPU protection
> + * @function_name: A string containing the function name for debug purposes
> + *   (usually __func__)
> + *
> + * @line: A line number where DC_FP_START was invoked for debug purpose
> + *   (usually __LINE__)
> + *
> + * This function is responsible for managing the use of kernel_fpu_begin() with
> + * the advantage of providing an event trace for debugging.
> + *
> + * Note: Do not call this function directly; always use DC_FP_START().
> + */
> +void dc_fpu_begin(const char *function_name, const int line)
> +{
> +	TRACE_DCN_FPU(true, function_name, line);
> +	kernel_fpu_begin();
> +}
> +
> +/**
> + * dc_fpu_end - Disable FPU protection
> + * @function_name: A string containing the function name for debug purposes
> + * @line: A-line number where DC_FP_END was invoked for debug purpose
> + *
> + * This function is responsible for managing the use of kernel_fpu_end() with
> + * the advantage of providing an event trace for debugging.
> + *
> + * Note: Do not call this function directly; always use DC_FP_END().
> + */
> +void dc_fpu_end(const char *function_name, const int line)
> +{
> +	TRACE_DCN_FPU(false, function_name, line);
> +	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
> new file mode 100644
> index 000000000000..fb54983c5c60
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#ifndef __DC_FPU_H__
> +#define __DC_FPU_H__
> +
> +void dc_fpu_begin(const char *function_name, const int line);
> +void dc_fpu_end(const char *function_name, const int line);
> +
> +#endif /* __DC_FPU_H__ */
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> index d2615357269b..d598ba697e45 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> @@ -37,3 +37,6 @@
>   
>   #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
>   	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
> +
> +#define TRACE_DCN_FPU(begin, function, line) \
> +	trace_dcn_fpu(begin, function, line)
> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
> index 126c2f3a4dd3..2ba49aef370d 100644
> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> @@ -52,9 +52,9 @@
>   
>   #if defined(CONFIG_DRM_AMD_DC_DCN)
>   #if defined(CONFIG_X86)
> -#include <asm/fpu/api.h>
> -#define DC_FP_START() kernel_fpu_begin()
> -#define DC_FP_END() kernel_fpu_end()
> +#include "amdgpu_dm/dc_fpu.h"
> +#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
> +#define DC_FP_END() dc_fpu_end(__func__, __LINE__)

This still has the issue that you don't correctly handle other 
architectures than x86.

I'm pretty sure the kernel bots will complain about that.

Regards,
Christian.

>   #elif defined(CONFIG_PPC64)
>   #include <asm/switch_to.h>
>   #include <asm/cputable.h>

Re: [PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Christian König]

Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> We don't have any mechanism for tracing FPU operations inside the
> display core, making the debug work a little bit tricky. This commit
> introduces a trace mechanism inside our DC_FP_START/END macros for
> trying to alleviate this problem.
>
> Changes since V2:
> - Make sure that we compile FPU operation only in the context that DCN
>    is enabled.
>
> 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/Makefile    |  4 ++
>   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
>   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
>   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
>   drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
>   drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
>   6 files changed, 128 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> index 91fb72c96545..718e123a3230 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> @@ -27,6 +27,10 @@
>   
>   AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
>   
> +ifdef CONFIG_DRM_AMD_DC_DCN
> +AMDGPUDM += dc_fpu.o
> +endif
> +
>   ifneq ($(CONFIG_DRM_AMD_DC),)
>   AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
>   endif
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> index 46a33f64cf8e..230bb12c405e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> @@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
>   		  __entry->refresh_rate_ns)
>   );
>   
> +TRACE_EVENT(dcn_fpu,
> +	    TP_PROTO(bool begin, const char *function, const int line),
> +	    TP_ARGS(begin, function, line),
> +
> +	    TP_STRUCT__entry(
> +			     __field(bool, begin)
> +			     __field(const char *, function)
> +			     __field(int, line)
> +	    ),
> +	    TP_fast_assign(
> +			   __entry->begin = begin;
> +			   __entry->function = function;
> +			   __entry->line = line;
> +	    ),
> +	    TP_printk("%s()+%d: %s",
> +		      __entry->function,
> +		      __entry->line,
> +		      __entry->begin ? "begin" : "end"
> +	    )
> +);
> +
>   #endif /* _AMDGPU_DM_TRACE_H_ */
>   
>   #undef TRACE_INCLUDE_PATH
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> new file mode 100644
> index 000000000000..d5d156a4517e
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#include "dc_trace.h"
> +
> +#include <asm/fpu/api.h>
> +
> +/**
> + * dc_fpu_begin - Enables FPU protection
> + * @function_name: A string containing the function name for debug purposes
> + *   (usually __func__)
> + *
> + * @line: A line number where DC_FP_START was invoked for debug purpose
> + *   (usually __LINE__)
> + *
> + * This function is responsible for managing the use of kernel_fpu_begin() with
> + * the advantage of providing an event trace for debugging.
> + *
> + * Note: Do not call this function directly; always use DC_FP_START().
> + */
> +void dc_fpu_begin(const char *function_name, const int line)
> +{
> +	TRACE_DCN_FPU(true, function_name, line);
> +	kernel_fpu_begin();
> +}
> +
> +/**
> + * dc_fpu_end - Disable FPU protection
> + * @function_name: A string containing the function name for debug purposes
> + * @line: A-line number where DC_FP_END was invoked for debug purpose
> + *
> + * This function is responsible for managing the use of kernel_fpu_end() with
> + * the advantage of providing an event trace for debugging.
> + *
> + * Note: Do not call this function directly; always use DC_FP_END().
> + */
> +void dc_fpu_end(const char *function_name, const int line)
> +{
> +	TRACE_DCN_FPU(false, function_name, line);
> +	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
> new file mode 100644
> index 000000000000..fb54983c5c60
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2021 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: AMD
> + *
> + */
> +
> +#ifndef __DC_FPU_H__
> +#define __DC_FPU_H__
> +
> +void dc_fpu_begin(const char *function_name, const int line);
> +void dc_fpu_end(const char *function_name, const int line);
> +
> +#endif /* __DC_FPU_H__ */
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> index d2615357269b..d598ba697e45 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> @@ -37,3 +37,6 @@
>   
>   #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
>   	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
> +
> +#define TRACE_DCN_FPU(begin, function, line) \
> +	trace_dcn_fpu(begin, function, line)
> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
> index 126c2f3a4dd3..2ba49aef370d 100644
> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> @@ -52,9 +52,9 @@
>   
>   #if defined(CONFIG_DRM_AMD_DC_DCN)
>   #if defined(CONFIG_X86)
> -#include <asm/fpu/api.h>
> -#define DC_FP_START() kernel_fpu_begin()
> -#define DC_FP_END() kernel_fpu_end()
> +#include "amdgpu_dm/dc_fpu.h"
> +#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
> +#define DC_FP_END() dc_fpu_end(__func__, __LINE__)

This still has the issue that you don't correctly handle other 
architectures than x86.

I'm pretty sure the kernel bots will complain about that.

Regards,
Christian.

>   #elif defined(CONFIG_PPC64)
>   #include <asm/switch_to.h>
>   #include <asm/cputable.h>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

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];
>   

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

[Christian König]

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];
>   

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 3/4] drm/amd/display: Add control mechanism for FPU utilization

[Christian König]

Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> DC invokes DC_FPU_START/END in multiple parts of the code; this can
> create a situation where we invoke this FPU operation in a nested way or
> exit too early. For avoiding this situation, this commit adds a
> mechanism where dc_fpu_begin/end manages the access to
> kernel_fpu_begin/end.
>
> Change since V2:
> - Christian: Do not use this_cpu_* between get/put_cpu_ptr().
>
> Change since V1:
> - Use a better variable names
> - Use get_cpu_ptr and put_cpu_ptr to better balance preemption enable
> and disable
>
> 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>
> ---
>   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 13 ++++---
>   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 36 ++++++++++++++++---
>   drivers/gpu/drm/amd/display/dc/dc_trace.h     |  4 +--
>   3 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> index 230bb12c405e..fdcaea22b456 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> @@ -638,23 +638,26 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
>   );
>   
>   TRACE_EVENT(dcn_fpu,
> -	    TP_PROTO(bool begin, const char *function, const int line),
> -	    TP_ARGS(begin, function, line),
> +	    TP_PROTO(bool begin, const char *function, const int line, const int recursion_depth),
> +	    TP_ARGS(begin, function, line, recursion_depth),
>   
>   	    TP_STRUCT__entry(
>   			     __field(bool, begin)
>   			     __field(const char *, function)
>   			     __field(int, line)
> +			     __field(int, recursion_depth)
>   	    ),
>   	    TP_fast_assign(
>   			   __entry->begin = begin;
>   			   __entry->function = function;
>   			   __entry->line = line;
> +			   __entry->recursion_depth = recursion_depth;
>   	    ),
> -	    TP_printk("%s()+%d: %s",
> +	    TP_printk("%s: recursion_depth: %d: %s()+%d:",
> +		      __entry->begin ? "begin" : "end",
> +		      __entry->recursion_depth,
>   		      __entry->function,
> -		      __entry->line,
> -		      __entry->begin ? "begin" : "end"
> +		      __entry->line
>   	    )
>   );
>   
> 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 d5d156a4517e..d0d3e8a34db5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> @@ -28,6 +28,19 @@
>   
>   #include <asm/fpu/api.h>
>   
> +/**
> + * DOC: DC FPU manipulation overview
> + *
> + * DC core uses FPU operations in multiple parts of the code, which requires a
> + * more specialized way to manage these areas' entrance. To fulfill this
> + * requirement, we created some wrapper functions that encapsulate
> + * kernel_fpu_begin/end to better fit our need in the display component. In
> + * summary, in this file, you can find functions related to FPU operation
> + * management.
> + */
> +
> +static DEFINE_PER_CPU(int, fpu_recursion_depth);
> +
>   /**
>    * dc_fpu_begin - Enables FPU protection
>    * @function_name: A string containing the function name for debug purposes
> @@ -43,8 +56,16 @@
>    */
>   void dc_fpu_begin(const char *function_name, const int line)
>   {
> -	TRACE_DCN_FPU(true, function_name, line);
> -	kernel_fpu_begin();
> +	int *pcpu;
> +
> +	pcpu = get_cpu_ptr(&fpu_recursion_depth);
> +	*pcpu += 1;
> +

This needs:

#if defined(CONFIG_X86)

> +	if (*pcpu == 1)
> +		kernel_fpu_begin();

#elif defined(CONFIG_PPC64)
....


Should I pick up the patches and help a bit with the non x86 support?

Regards,
Christian.

> +
> +	TRACE_DCN_FPU(true, function_name, line, *pcpu);
> +	put_cpu_ptr(&fpu_recursion_depth);
>   }
>   
>   /**
> @@ -59,6 +80,13 @@ void dc_fpu_begin(const char *function_name, const int line)
>    */
>   void dc_fpu_end(const char *function_name, const int line)
>   {
> -	TRACE_DCN_FPU(false, function_name, line);
> -	kernel_fpu_end();
> +	int *pcpu;
> +
> +	pcpu = get_cpu_ptr(&fpu_recursion_depth);
> +	*pcpu -= 1;
> +	if (*pcpu <= 0)
> +		kernel_fpu_end();
> +
> +	TRACE_DCN_FPU(false, function_name, line, *pcpu);
> +	put_cpu_ptr(&fpu_recursion_depth);
>   }
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> index d598ba697e45..c711797e5c9e 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> @@ -38,5 +38,5 @@
>   #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
>   	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
>   
> -#define TRACE_DCN_FPU(begin, function, line) \
> -	trace_dcn_fpu(begin, function, line)
> +#define TRACE_DCN_FPU(begin, function, line, ref_count) \
> +	trace_dcn_fpu(begin, function, line, ref_count)

Re: [PATCH v3 3/4] drm/amd/display: Add control mechanism for FPU utilization

[Christian König]

Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> DC invokes DC_FPU_START/END in multiple parts of the code; this can
> create a situation where we invoke this FPU operation in a nested way or
> exit too early. For avoiding this situation, this commit adds a
> mechanism where dc_fpu_begin/end manages the access to
> kernel_fpu_begin/end.
>
> Change since V2:
> - Christian: Do not use this_cpu_* between get/put_cpu_ptr().
>
> Change since V1:
> - Use a better variable names
> - Use get_cpu_ptr and put_cpu_ptr to better balance preemption enable
> and disable
>
> 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>
> ---
>   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 13 ++++---
>   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 36 ++++++++++++++++---
>   drivers/gpu/drm/amd/display/dc/dc_trace.h     |  4 +--
>   3 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> index 230bb12c405e..fdcaea22b456 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> @@ -638,23 +638,26 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
>   );
>   
>   TRACE_EVENT(dcn_fpu,
> -	    TP_PROTO(bool begin, const char *function, const int line),
> -	    TP_ARGS(begin, function, line),
> +	    TP_PROTO(bool begin, const char *function, const int line, const int recursion_depth),
> +	    TP_ARGS(begin, function, line, recursion_depth),
>   
>   	    TP_STRUCT__entry(
>   			     __field(bool, begin)
>   			     __field(const char *, function)
>   			     __field(int, line)
> +			     __field(int, recursion_depth)
>   	    ),
>   	    TP_fast_assign(
>   			   __entry->begin = begin;
>   			   __entry->function = function;
>   			   __entry->line = line;
> +			   __entry->recursion_depth = recursion_depth;
>   	    ),
> -	    TP_printk("%s()+%d: %s",
> +	    TP_printk("%s: recursion_depth: %d: %s()+%d:",
> +		      __entry->begin ? "begin" : "end",
> +		      __entry->recursion_depth,
>   		      __entry->function,
> -		      __entry->line,
> -		      __entry->begin ? "begin" : "end"
> +		      __entry->line
>   	    )
>   );
>   
> 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 d5d156a4517e..d0d3e8a34db5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> @@ -28,6 +28,19 @@
>   
>   #include <asm/fpu/api.h>
>   
> +/**
> + * DOC: DC FPU manipulation overview
> + *
> + * DC core uses FPU operations in multiple parts of the code, which requires a
> + * more specialized way to manage these areas' entrance. To fulfill this
> + * requirement, we created some wrapper functions that encapsulate
> + * kernel_fpu_begin/end to better fit our need in the display component. In
> + * summary, in this file, you can find functions related to FPU operation
> + * management.
> + */
> +
> +static DEFINE_PER_CPU(int, fpu_recursion_depth);
> +
>   /**
>    * dc_fpu_begin - Enables FPU protection
>    * @function_name: A string containing the function name for debug purposes
> @@ -43,8 +56,16 @@
>    */
>   void dc_fpu_begin(const char *function_name, const int line)
>   {
> -	TRACE_DCN_FPU(true, function_name, line);
> -	kernel_fpu_begin();
> +	int *pcpu;
> +
> +	pcpu = get_cpu_ptr(&fpu_recursion_depth);
> +	*pcpu += 1;
> +

This needs:

#if defined(CONFIG_X86)

> +	if (*pcpu == 1)
> +		kernel_fpu_begin();

#elif defined(CONFIG_PPC64)
....


Should I pick up the patches and help a bit with the non x86 support?

Regards,
Christian.

> +
> +	TRACE_DCN_FPU(true, function_name, line, *pcpu);
> +	put_cpu_ptr(&fpu_recursion_depth);
>   }
>   
>   /**
> @@ -59,6 +80,13 @@ void dc_fpu_begin(const char *function_name, const int line)
>    */
>   void dc_fpu_end(const char *function_name, const int line)
>   {
> -	TRACE_DCN_FPU(false, function_name, line);
> -	kernel_fpu_end();
> +	int *pcpu;
> +
> +	pcpu = get_cpu_ptr(&fpu_recursion_depth);
> +	*pcpu -= 1;
> +	if (*pcpu <= 0)
> +		kernel_fpu_end();
> +
> +	TRACE_DCN_FPU(false, function_name, line, *pcpu);
> +	put_cpu_ptr(&fpu_recursion_depth);
>   }
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> index d598ba697e45..c711797e5c9e 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> @@ -38,5 +38,5 @@
>   #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
>   	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
>   
> -#define TRACE_DCN_FPU(begin, function, line) \
> -	trace_dcn_fpu(begin, function, line)
> +#define TRACE_DCN_FPU(begin, function, line, ref_count) \
> +	trace_dcn_fpu(begin, function, line, ref_count)

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Rodrigo Siqueira]

On 07/20, Christian König wrote:
> 
> 
> Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> > We don't have any mechanism for tracing FPU operations inside the
> > display core, making the debug work a little bit tricky. This commit
> > introduces a trace mechanism inside our DC_FP_START/END macros for
> > trying to alleviate this problem.
> > 
> > Changes since V2:
> > - Make sure that we compile FPU operation only in the context that DCN
> >    is enabled.
> > 
> > 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/Makefile    |  4 ++
> >   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
> >   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
> >   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
> >   drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
> >   drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
> >   6 files changed, 128 insertions(+), 3 deletions(-)
> >   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> >   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > index 91fb72c96545..718e123a3230 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > @@ -27,6 +27,10 @@
> >   AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
> > +ifdef CONFIG_DRM_AMD_DC_DCN
> > +AMDGPUDM += dc_fpu.o
> > +endif
> > +
> >   ifneq ($(CONFIG_DRM_AMD_DC),)
> >   AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
> >   endif
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > index 46a33f64cf8e..230bb12c405e 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > @@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
> >   		  __entry->refresh_rate_ns)
> >   );
> > +TRACE_EVENT(dcn_fpu,
> > +	    TP_PROTO(bool begin, const char *function, const int line),
> > +	    TP_ARGS(begin, function, line),
> > +
> > +	    TP_STRUCT__entry(
> > +			     __field(bool, begin)
> > +			     __field(const char *, function)
> > +			     __field(int, line)
> > +	    ),
> > +	    TP_fast_assign(
> > +			   __entry->begin = begin;
> > +			   __entry->function = function;
> > +			   __entry->line = line;
> > +	    ),
> > +	    TP_printk("%s()+%d: %s",
> > +		      __entry->function,
> > +		      __entry->line,
> > +		      __entry->begin ? "begin" : "end"
> > +	    )
> > +);
> > +
> >   #endif /* _AMDGPU_DM_TRACE_H_ */
> >   #undef TRACE_INCLUDE_PATH
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> > new file mode 100644
> > index 000000000000..d5d156a4517e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright 2021 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + *
> > + * Authors: AMD
> > + *
> > + */
> > +
> > +#include "dc_trace.h"
> > +
> > +#include <asm/fpu/api.h>
> > +
> > +/**
> > + * dc_fpu_begin - Enables FPU protection
> > + * @function_name: A string containing the function name for debug purposes
> > + *   (usually __func__)
> > + *
> > + * @line: A line number where DC_FP_START was invoked for debug purpose
> > + *   (usually __LINE__)
> > + *
> > + * This function is responsible for managing the use of kernel_fpu_begin() with
> > + * the advantage of providing an event trace for debugging.
> > + *
> > + * Note: Do not call this function directly; always use DC_FP_START().
> > + */
> > +void dc_fpu_begin(const char *function_name, const int line)
> > +{
> > +	TRACE_DCN_FPU(true, function_name, line);
> > +	kernel_fpu_begin();
> > +}
> > +
> > +/**
> > + * dc_fpu_end - Disable FPU protection
> > + * @function_name: A string containing the function name for debug purposes
> > + * @line: A-line number where DC_FP_END was invoked for debug purpose
> > + *
> > + * This function is responsible for managing the use of kernel_fpu_end() with
> > + * the advantage of providing an event trace for debugging.
> > + *
> > + * Note: Do not call this function directly; always use DC_FP_END().
> > + */
> > +void dc_fpu_end(const char *function_name, const int line)
> > +{
> > +	TRACE_DCN_FPU(false, function_name, line);
> > +	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
> > new file mode 100644
> > index 000000000000..fb54983c5c60
> > --- /dev/null
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright 2021 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + *
> > + * Authors: AMD
> > + *
> > + */
> > +
> > +#ifndef __DC_FPU_H__
> > +#define __DC_FPU_H__
> > +
> > +void dc_fpu_begin(const char *function_name, const int line);
> > +void dc_fpu_end(const char *function_name, const int line);
> > +
> > +#endif /* __DC_FPU_H__ */
> > diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> > index d2615357269b..d598ba697e45 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
> > +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> > @@ -37,3 +37,6 @@
> >   #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
> >   	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
> > +
> > +#define TRACE_DCN_FPU(begin, function, line) \
> > +	trace_dcn_fpu(begin, function, line)
> > diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
> > index 126c2f3a4dd3..2ba49aef370d 100644
> > --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> > +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> > @@ -52,9 +52,9 @@
> >   #if defined(CONFIG_DRM_AMD_DC_DCN)
> >   #if defined(CONFIG_X86)
> > -#include <asm/fpu/api.h>
> > -#define DC_FP_START() kernel_fpu_begin()
> > -#define DC_FP_END() kernel_fpu_end()
> > +#include "amdgpu_dm/dc_fpu.h"
> > +#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
> > +#define DC_FP_END() dc_fpu_end(__func__, __LINE__)
> 
> This still has the issue that you don't correctly handle other architectures
> than x86.
> 
> I'm pretty sure the kernel bots will complain about that.

Hi Christian,

Kernel bots will not break because we are using CONFIG_DRM_AMD_DC_DCN to
isolate DCN architecture. Currently, we support DCN on x86 and PPC (idk
the details for ppc); in this series, I'm trying to enable us to start
to isolate FPU code to a single place. When we start to move the FPU
functions to a single place, we can take the opportunity to refactor the
FPU code. After we isolate the FPU source, we can safely start to enable
other architectures such as ARM.

This is why I did not try to enable it for other architecture yet. I
think this is the second step.

Thanks
Siqueira
 
> Regards,
> Christian.
> 
> >   #elif defined(CONFIG_PPC64)
> >   #include <asm/switch_to.h>
> >   #include <asm/cputable.h>
> 

-- 
Rodrigo Siqueira
https://siqueira.tech

Re: [PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Rodrigo Siqueira]

On 07/20, Christian König wrote:
> 
> 
> Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
> > We don't have any mechanism for tracing FPU operations inside the
> > display core, making the debug work a little bit tricky. This commit
> > introduces a trace mechanism inside our DC_FP_START/END macros for
> > trying to alleviate this problem.
> > 
> > Changes since V2:
> > - Make sure that we compile FPU operation only in the context that DCN
> >    is enabled.
> > 
> > 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/Makefile    |  4 ++
> >   .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
> >   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
> >   .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
> >   drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
> >   drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
> >   6 files changed, 128 insertions(+), 3 deletions(-)
> >   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> >   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > index 91fb72c96545..718e123a3230 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > @@ -27,6 +27,10 @@
> >   AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
> > +ifdef CONFIG_DRM_AMD_DC_DCN
> > +AMDGPUDM += dc_fpu.o
> > +endif
> > +
> >   ifneq ($(CONFIG_DRM_AMD_DC),)
> >   AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
> >   endif
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > index 46a33f64cf8e..230bb12c405e 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
> > @@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
> >   		  __entry->refresh_rate_ns)
> >   );
> > +TRACE_EVENT(dcn_fpu,
> > +	    TP_PROTO(bool begin, const char *function, const int line),
> > +	    TP_ARGS(begin, function, line),
> > +
> > +	    TP_STRUCT__entry(
> > +			     __field(bool, begin)
> > +			     __field(const char *, function)
> > +			     __field(int, line)
> > +	    ),
> > +	    TP_fast_assign(
> > +			   __entry->begin = begin;
> > +			   __entry->function = function;
> > +			   __entry->line = line;
> > +	    ),
> > +	    TP_printk("%s()+%d: %s",
> > +		      __entry->function,
> > +		      __entry->line,
> > +		      __entry->begin ? "begin" : "end"
> > +	    )
> > +);
> > +
> >   #endif /* _AMDGPU_DM_TRACE_H_ */
> >   #undef TRACE_INCLUDE_PATH
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> > new file mode 100644
> > index 000000000000..d5d156a4517e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright 2021 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + *
> > + * Authors: AMD
> > + *
> > + */
> > +
> > +#include "dc_trace.h"
> > +
> > +#include <asm/fpu/api.h>
> > +
> > +/**
> > + * dc_fpu_begin - Enables FPU protection
> > + * @function_name: A string containing the function name for debug purposes
> > + *   (usually __func__)
> > + *
> > + * @line: A line number where DC_FP_START was invoked for debug purpose
> > + *   (usually __LINE__)
> > + *
> > + * This function is responsible for managing the use of kernel_fpu_begin() with
> > + * the advantage of providing an event trace for debugging.
> > + *
> > + * Note: Do not call this function directly; always use DC_FP_START().
> > + */
> > +void dc_fpu_begin(const char *function_name, const int line)
> > +{
> > +	TRACE_DCN_FPU(true, function_name, line);
> > +	kernel_fpu_begin();
> > +}
> > +
> > +/**
> > + * dc_fpu_end - Disable FPU protection
> > + * @function_name: A string containing the function name for debug purposes
> > + * @line: A-line number where DC_FP_END was invoked for debug purpose
> > + *
> > + * This function is responsible for managing the use of kernel_fpu_end() with
> > + * the advantage of providing an event trace for debugging.
> > + *
> > + * Note: Do not call this function directly; always use DC_FP_END().
> > + */
> > +void dc_fpu_end(const char *function_name, const int line)
> > +{
> > +	TRACE_DCN_FPU(false, function_name, line);
> > +	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
> > new file mode 100644
> > index 000000000000..fb54983c5c60
> > --- /dev/null
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright 2021 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + *
> > + * Authors: AMD
> > + *
> > + */
> > +
> > +#ifndef __DC_FPU_H__
> > +#define __DC_FPU_H__
> > +
> > +void dc_fpu_begin(const char *function_name, const int line);
> > +void dc_fpu_end(const char *function_name, const int line);
> > +
> > +#endif /* __DC_FPU_H__ */
> > diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> > index d2615357269b..d598ba697e45 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
> > +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
> > @@ -37,3 +37,6 @@
> >   #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
> >   	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
> > +
> > +#define TRACE_DCN_FPU(begin, function, line) \
> > +	trace_dcn_fpu(begin, function, line)
> > diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
> > index 126c2f3a4dd3..2ba49aef370d 100644
> > --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> > +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> > @@ -52,9 +52,9 @@
> >   #if defined(CONFIG_DRM_AMD_DC_DCN)
> >   #if defined(CONFIG_X86)
> > -#include <asm/fpu/api.h>
> > -#define DC_FP_START() kernel_fpu_begin()
> > -#define DC_FP_END() kernel_fpu_end()
> > +#include "amdgpu_dm/dc_fpu.h"
> > +#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
> > +#define DC_FP_END() dc_fpu_end(__func__, __LINE__)
> 
> This still has the issue that you don't correctly handle other architectures
> than x86.
> 
> I'm pretty sure the kernel bots will complain about that.

Hi Christian,

Kernel bots will not break because we are using CONFIG_DRM_AMD_DC_DCN to
isolate DCN architecture. Currently, we support DCN on x86 and PPC (idk
the details for ppc); in this series, I'm trying to enable us to start
to isolate FPU code to a single place. When we start to move the FPU
functions to a single place, we can take the opportunity to refactor the
FPU code. After we isolate the FPU source, we can safely start to enable
other architectures such as ARM.

This is why I did not try to enable it for other architecture yet. I
think this is the second step.

Thanks
Siqueira
 
> Regards,
> Christian.
> 
> >   #elif defined(CONFIG_PPC64)
> >   #include <asm/switch_to.h>
> >   #include <asm/cputable.h>
> 

-- 
Rodrigo Siqueira
https://siqueira.tech
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Christian König]

Am 21.07.21 um 13:44 schrieb Rodrigo Siqueira:
> On 07/20, Christian König wrote:
>>
>> Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
>>> We don't have any mechanism for tracing FPU operations inside the
>>> display core, making the debug work a little bit tricky. This commit
>>> introduces a trace mechanism inside our DC_FP_START/END macros for
>>> trying to alleviate this problem.
>>>
>>> Changes since V2:
>>> - Make sure that we compile FPU operation only in the context that DCN
>>>     is enabled.
>>>
>>> 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/Makefile    |  4 ++
>>>    .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
>>>    drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
>>>    drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
>>>    6 files changed, 128 insertions(+), 3 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> index 91fb72c96545..718e123a3230 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> @@ -27,6 +27,10 @@
>>>    AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
>>> +ifdef CONFIG_DRM_AMD_DC_DCN
>>> +AMDGPUDM += dc_fpu.o
>>> +endif
>>> +
>>>    ifneq ($(CONFIG_DRM_AMD_DC),)
>>>    AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
>>>    endif
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> index 46a33f64cf8e..230bb12c405e 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> @@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
>>>    		  __entry->refresh_rate_ns)
>>>    );
>>> +TRACE_EVENT(dcn_fpu,
>>> +	    TP_PROTO(bool begin, const char *function, const int line),
>>> +	    TP_ARGS(begin, function, line),
>>> +
>>> +	    TP_STRUCT__entry(
>>> +			     __field(bool, begin)
>>> +			     __field(const char *, function)
>>> +			     __field(int, line)
>>> +	    ),
>>> +	    TP_fast_assign(
>>> +			   __entry->begin = begin;
>>> +			   __entry->function = function;
>>> +			   __entry->line = line;
>>> +	    ),
>>> +	    TP_printk("%s()+%d: %s",
>>> +		      __entry->function,
>>> +		      __entry->line,
>>> +		      __entry->begin ? "begin" : "end"
>>> +	    )
>>> +);
>>> +
>>>    #endif /* _AMDGPU_DM_TRACE_H_ */
>>>    #undef TRACE_INCLUDE_PATH
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>> new file mode 100644
>>> index 000000000000..d5d156a4517e
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>> @@ -0,0 +1,64 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright 2021 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + * Authors: AMD
>>> + *
>>> + */
>>> +
>>> +#include "dc_trace.h"
>>> +
>>> +#include <asm/fpu/api.h>
>>> +
>>> +/**
>>> + * dc_fpu_begin - Enables FPU protection
>>> + * @function_name: A string containing the function name for debug purposes
>>> + *   (usually __func__)
>>> + *
>>> + * @line: A line number where DC_FP_START was invoked for debug purpose
>>> + *   (usually __LINE__)
>>> + *
>>> + * This function is responsible for managing the use of kernel_fpu_begin() with
>>> + * the advantage of providing an event trace for debugging.
>>> + *
>>> + * Note: Do not call this function directly; always use DC_FP_START().
>>> + */
>>> +void dc_fpu_begin(const char *function_name, const int line)
>>> +{
>>> +	TRACE_DCN_FPU(true, function_name, line);
>>> +	kernel_fpu_begin();
>>> +}
>>> +
>>> +/**
>>> + * dc_fpu_end - Disable FPU protection
>>> + * @function_name: A string containing the function name for debug purposes
>>> + * @line: A-line number where DC_FP_END was invoked for debug purpose
>>> + *
>>> + * This function is responsible for managing the use of kernel_fpu_end() with
>>> + * the advantage of providing an event trace for debugging.
>>> + *
>>> + * Note: Do not call this function directly; always use DC_FP_END().
>>> + */
>>> +void dc_fpu_end(const char *function_name, const int line)
>>> +{
>>> +	TRACE_DCN_FPU(false, function_name, line);
>>> +	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
>>> new file mode 100644
>>> index 000000000000..fb54983c5c60
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>>> @@ -0,0 +1,33 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright 2021 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + * Authors: AMD
>>> + *
>>> + */
>>> +
>>> +#ifndef __DC_FPU_H__
>>> +#define __DC_FPU_H__
>>> +
>>> +void dc_fpu_begin(const char *function_name, const int line);
>>> +void dc_fpu_end(const char *function_name, const int line);
>>> +
>>> +#endif /* __DC_FPU_H__ */
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> index d2615357269b..d598ba697e45 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> @@ -37,3 +37,6 @@
>>>    #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
>>>    	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
>>> +
>>> +#define TRACE_DCN_FPU(begin, function, line) \
>>> +	trace_dcn_fpu(begin, function, line)
>>> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> index 126c2f3a4dd3..2ba49aef370d 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> @@ -52,9 +52,9 @@
>>>    #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>    #if defined(CONFIG_X86)
>>> -#include <asm/fpu/api.h>
>>> -#define DC_FP_START() kernel_fpu_begin()
>>> -#define DC_FP_END() kernel_fpu_end()
>>> +#include "amdgpu_dm/dc_fpu.h"
>>> +#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
>>> +#define DC_FP_END() dc_fpu_end(__func__, __LINE__)
>> This still has the issue that you don't correctly handle other architectures
>> than x86.
>>
>> I'm pretty sure the kernel bots will complain about that.
> Hi Christian,
>
> Kernel bots will not break because we are using CONFIG_DRM_AMD_DC_DCN to
> isolate DCN architecture. Currently, we support DCN on x86 and PPC (idk
> the details for ppc); in this series, I'm trying to enable us to start
> to isolate FPU code to a single place. When we start to move the FPU
> functions to a single place, we can take the opportunity to refactor the
> FPU code. After we isolate the FPU source, we can safely start to enable
> other architectures such as ARM.
>
> This is why I did not try to enable it for other architecture yet. I
> think this is the second step.

That sounds like a plan to me, but I have to tell you that this will 
still break the build bots for PPC.

>
> Thanks
> Siqueira
>   
>> Regards,
>> Christian.
>>
>>>    #elif defined(CONFIG_PPC64)

Because of this switch here.

You need the unification of the floating point handling from the other 
mail thread as prerequisite for this series if you want to avoid this.

Regards,
Christian.

>>>    #include <asm/switch_to.h>
>>>    #include <asm/cputable.h>

Re: [PATCH v3 2/4] drm/amd/display: Add FPU event trace

[Christian König]

Am 21.07.21 um 13:44 schrieb Rodrigo Siqueira:
> On 07/20, Christian König wrote:
>>
>> Am 20.07.21 um 02:49 schrieb Rodrigo Siqueira:
>>> We don't have any mechanism for tracing FPU operations inside the
>>> display core, making the debug work a little bit tricky. This commit
>>> introduces a trace mechanism inside our DC_FP_START/END macros for
>>> trying to alleviate this problem.
>>>
>>> Changes since V2:
>>> - Make sure that we compile FPU operation only in the context that DCN
>>>     is enabled.
>>>
>>> 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/Makefile    |  4 ++
>>>    .../amd/display/amdgpu_dm/amdgpu_dm_trace.h   | 21 ++++++
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.c    | 64 +++++++++++++++++++
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_fpu.h    | 33 ++++++++++
>>>    drivers/gpu/drm/amd/display/dc/dc_trace.h     |  3 +
>>>    drivers/gpu/drm/amd/display/dc/os_types.h     |  6 +-
>>>    6 files changed, 128 insertions(+), 3 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> index 91fb72c96545..718e123a3230 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> @@ -27,6 +27,10 @@
>>>    AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
>>> +ifdef CONFIG_DRM_AMD_DC_DCN
>>> +AMDGPUDM += dc_fpu.o
>>> +endif
>>> +
>>>    ifneq ($(CONFIG_DRM_AMD_DC),)
>>>    AMDGPUDM += amdgpu_dm_services.o amdgpu_dm_helpers.o amdgpu_dm_pp_smu.o amdgpu_dm_psr.o
>>>    endif
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> index 46a33f64cf8e..230bb12c405e 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_trace.h
>>> @@ -637,6 +637,27 @@ TRACE_EVENT(amdgpu_refresh_rate_track,
>>>    		  __entry->refresh_rate_ns)
>>>    );
>>> +TRACE_EVENT(dcn_fpu,
>>> +	    TP_PROTO(bool begin, const char *function, const int line),
>>> +	    TP_ARGS(begin, function, line),
>>> +
>>> +	    TP_STRUCT__entry(
>>> +			     __field(bool, begin)
>>> +			     __field(const char *, function)
>>> +			     __field(int, line)
>>> +	    ),
>>> +	    TP_fast_assign(
>>> +			   __entry->begin = begin;
>>> +			   __entry->function = function;
>>> +			   __entry->line = line;
>>> +	    ),
>>> +	    TP_printk("%s()+%d: %s",
>>> +		      __entry->function,
>>> +		      __entry->line,
>>> +		      __entry->begin ? "begin" : "end"
>>> +	    )
>>> +);
>>> +
>>>    #endif /* _AMDGPU_DM_TRACE_H_ */
>>>    #undef TRACE_INCLUDE_PATH
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>> new file mode 100644
>>> index 000000000000..d5d156a4517e
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c
>>> @@ -0,0 +1,64 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright 2021 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + * Authors: AMD
>>> + *
>>> + */
>>> +
>>> +#include "dc_trace.h"
>>> +
>>> +#include <asm/fpu/api.h>
>>> +
>>> +/**
>>> + * dc_fpu_begin - Enables FPU protection
>>> + * @function_name: A string containing the function name for debug purposes
>>> + *   (usually __func__)
>>> + *
>>> + * @line: A line number where DC_FP_START was invoked for debug purpose
>>> + *   (usually __LINE__)
>>> + *
>>> + * This function is responsible for managing the use of kernel_fpu_begin() with
>>> + * the advantage of providing an event trace for debugging.
>>> + *
>>> + * Note: Do not call this function directly; always use DC_FP_START().
>>> + */
>>> +void dc_fpu_begin(const char *function_name, const int line)
>>> +{
>>> +	TRACE_DCN_FPU(true, function_name, line);
>>> +	kernel_fpu_begin();
>>> +}
>>> +
>>> +/**
>>> + * dc_fpu_end - Disable FPU protection
>>> + * @function_name: A string containing the function name for debug purposes
>>> + * @line: A-line number where DC_FP_END was invoked for debug purpose
>>> + *
>>> + * This function is responsible for managing the use of kernel_fpu_end() with
>>> + * the advantage of providing an event trace for debugging.
>>> + *
>>> + * Note: Do not call this function directly; always use DC_FP_END().
>>> + */
>>> +void dc_fpu_end(const char *function_name, const int line)
>>> +{
>>> +	TRACE_DCN_FPU(false, function_name, line);
>>> +	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
>>> new file mode 100644
>>> index 000000000000..fb54983c5c60
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.h
>>> @@ -0,0 +1,33 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright 2021 Advanced Micro Devices, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> + * OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + * Authors: AMD
>>> + *
>>> + */
>>> +
>>> +#ifndef __DC_FPU_H__
>>> +#define __DC_FPU_H__
>>> +
>>> +void dc_fpu_begin(const char *function_name, const int line);
>>> +void dc_fpu_end(const char *function_name, const int line);
>>> +
>>> +#endif /* __DC_FPU_H__ */
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_trace.h b/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> index d2615357269b..d598ba697e45 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/dc_trace.h
>>> @@ -37,3 +37,6 @@
>>>    #define TRACE_DCN_CLOCK_STATE(dcn_clocks) \
>>>    	trace_amdgpu_dm_dc_clocks_state(dcn_clocks)
>>> +
>>> +#define TRACE_DCN_FPU(begin, function, line) \
>>> +	trace_dcn_fpu(begin, function, line)
>>> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> index 126c2f3a4dd3..2ba49aef370d 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
>>> @@ -52,9 +52,9 @@
>>>    #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>    #if defined(CONFIG_X86)
>>> -#include <asm/fpu/api.h>
>>> -#define DC_FP_START() kernel_fpu_begin()
>>> -#define DC_FP_END() kernel_fpu_end()
>>> +#include "amdgpu_dm/dc_fpu.h"
>>> +#define DC_FP_START() dc_fpu_begin(__func__, __LINE__)
>>> +#define DC_FP_END() dc_fpu_end(__func__, __LINE__)
>> This still has the issue that you don't correctly handle other architectures
>> than x86.
>>
>> I'm pretty sure the kernel bots will complain about that.
> Hi Christian,
>
> Kernel bots will not break because we are using CONFIG_DRM_AMD_DC_DCN to
> isolate DCN architecture. Currently, we support DCN on x86 and PPC (idk
> the details for ppc); in this series, I'm trying to enable us to start
> to isolate FPU code to a single place. When we start to move the FPU
> functions to a single place, we can take the opportunity to refactor the
> FPU code. After we isolate the FPU source, we can safely start to enable
> other architectures such as ARM.
>
> This is why I did not try to enable it for other architecture yet. I
> think this is the second step.

That sounds like a plan to me, but I have to tell you that this will 
still break the build bots for PPC.

>
> Thanks
> Siqueira
>   
>> Regards,
>> Christian.
>>
>>>    #elif defined(CONFIG_PPC64)

Because of this switch here.

You need the unification of the floating point handling from the other 
mail thread as prerequisite for this series if you want to avoid this.

Regards,
Christian.

>>>    #include <asm/switch_to.h>
>>>    #include <asm/cputable.h>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx