Re: [PATCH 0/4] drm/amd/display: Base changes for isolating FPU operation in a single place

["Christian König" <christian.koenig@amd.com>]

Hi Rodrigo,

I'm not so happy about the whole recursion thing, but I think that is 
something which can be worked on later on.

Apart from that the approach sounds solid to me.

Regards,
Christian.

Am 31.03.21 um 14:24 schrieb 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 Windows; 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.
>
> # 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 the 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.
>
> # 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.
>
> 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 ref count control for FPU utilization
>    drm/amd/display: Add DC_FP helper to check FPU state
>
>   .../gpu/drm/amd/display/amdgpu_dm/Makefile    |   3 +-
>   .../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 | 106 +++++++++++++++++
>   .../drm/amd/display/dc/fpu_operations/dcn2x.h |  33 ++++++
>   drivers/gpu/drm/amd/display/dc/os_types.h     |   6 +-
>   13 files changed, 381 insertions(+), 43 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
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel