Re: [PATCH v4 1/4] drm/amd/display: Move specific DCN2x code that uses FPU to DML

[Christian König <christian.koenig@xxxxxxx>]

Am 27.07.21 um 02:52 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 inside DML for centralizing shared DCN
> functions that require FPU and have been used outside DML. For
> illustrating this process of transferring FPU functions to the DML
> folder, this commit 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 V3:
> - Jun: Instead of creating a new directory to keep the FPU code, let's
> make the DML folder the only part that requires FPU access. Drop
> fpu_operation folder.
> - Christian: Fix function code style.
>
> 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@xxxxxxx>
> Cc: Anson Jacob <Anson.Jacob@xxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Hersen Wu <hersenxs.wu@xxxxxxx>
> Cc: Aric Cyr <aric.cyr@xxxxxxx>
> Cc: Jun Lei <jun.lei@xxxxxxx>
> Cc: Dmytro Laktyushkin <dmytro.laktyushkin@xxxxxxx>
> Cc: Qingqing Zhuo <qingqing.zhuo@xxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>

BTW: The "Reported-by" tag is only used for bug fixes, but since this is 
a new series and only the test robot has complained about it on the 
mailing list you don't need to add it.

> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>

Reviewed-by: Christian König <christian.koenig@xxxxxxx>

Regards,
Christian.

> ---
>   .../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 +
>   drivers/gpu/drm/amd/display/dc/dml/Makefile   |  4 +
>   .../gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.c  | 84 +++++++++++++++++++
>   .../gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.h  | 34 ++++++++
>   6 files changed, 126 insertions(+), 39 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.c
>   create mode 100644 drivers/gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.h
>
> 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 0b1cd1dbed8b..988d7c02199c 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 "dml/dcn2x/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 f27fc2acac57..fbbdf9976183 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 "dml/dcn2x/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/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> index 45862167e6ce..56055df2e8d2 100644
> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> @@ -58,6 +58,8 @@ CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_lib.o := $(dml_ccflags)
>   
>   ifdef CONFIG_DRM_AMD_DC_DCN
>   CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_vba.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dcn2x/dcn2x.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := $(dml_ccflags)
>   CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := $(dml_ccflags)
>   CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_rq_dlg_calc_20.o := $(dml_ccflags)
>   CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20v2.o := $(dml_ccflags)
> @@ -70,6 +72,7 @@ CFLAGS_$(AMDDALPATH)/dc/dml/dcn31/display_mode_vba_31.o := $(dml_ccflags) $(fram
>   CFLAGS_$(AMDDALPATH)/dc/dml/dcn31/display_rq_dlg_calc_31.o := $(dml_ccflags)
>   CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_lib.o := $(dml_ccflags)
>   CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml/display_mode_vba.o := $(dml_rcflags)
> +CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml/dcn2x/dcn2x.o := $(dml_rcflags)
>   CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := $(dml_rcflags)
>   CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml/dcn20/display_rq_dlg_calc_20.o := $(dml_rcflags)
>   CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20v2.o := $(dml_rcflags)
> @@ -91,6 +94,7 @@ DML = display_mode_lib.o display_rq_dlg_helpers.o dml1_display_rq_dlg_calc.o \
>   
>   ifdef CONFIG_DRM_AMD_DC_DCN
>   DML += display_mode_vba.o dcn20/display_rq_dlg_calc_20.o dcn20/display_mode_vba_20.o
> +DML += dcn2x/dcn2x.o
>   DML += dcn20/display_rq_dlg_calc_20v2.o dcn20/display_mode_vba_20v2.o
>   DML += dcn21/display_rq_dlg_calc_21.o dcn21/display_mode_vba_21.o
>   DML += dcn30/display_mode_vba_30.o dcn30/display_rq_dlg_calc_30.o
> diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.c b/drivers/gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.c
> new file mode 100644
> index 000000000000..8f0f6220327d
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/dml/dcn2x/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 "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/dml/dcn2x/dcn2x.h b/drivers/gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.h
> new file mode 100644
> index 000000000000..331547ba0713
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/dml/dcn2x/dcn2x.h
> @@ -0,0 +1,34 @@
> +/* 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__ */