Re: [PATCH] drm/amd/display: Remove duplicate code for DCN314 DML calculation

From: Harry Wentland
Date: Fri Oct 21 2022 - 11:12:57 EST

Next message: Nícolas F. R. A. Prado: "Re: [PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func"
Previous message: Sean Christopherson: "Re: [PATCH] KVM: x86/mmu: Release the pfn in handle_abnormal_pfn() error path"
In reply to: Rafael Mendonca: "[PATCH] drm/amd/display: Remove duplicate code for DCN314 DML calculation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2022-10-20 18:10, Rafael Mendonca wrote:
> This is an extension of commit fd3bc691fc7b ("drm/amd/display: Remove
> duplicate code across dcn30 and dcn31"), which removed duplicate code for
> the function CalculateBytePerPixelAnd256BBlockSizes() across dcn30 and
> dcn31. At the time the aforementioned commit was introduced, support for
> DCN 3.1.4 was still not merged. Thus, this deletes duplicate code for
> CalculateBytePerPixelAnd256BBlockSizes(), that was introduced later in
> DCN314, in favor of using the respective functionality from
> 'display_mode_vba_30.h'.
>
> Additionally, by doing that, we also fix a duplicate argument issue
> reported by coccinelle in 'display_rq_dlg_calc_314.c':
>
> static bool CalculateBytePerPixelAnd256BBlockSizes(...) {
> ...
> } else if (SourcePixelFormat == dm_444_16 || SourcePixelFormat == dm_444_16) {
> ...
> }
>

A lot of the uglyness of this code and some of the duplicate nature
of it are due to the fact that this comes directly from the HW
designers and is consumed as HW gospel. Years ago we tried to write
our own beautiful code to implement the HW bandwidth constraints and
calculations. This proved problematic as we had an argument with
HW designers each time there was a bug and first had to try to
prove that our own code was good. Consuming their code (more-or-less)
as-is short circuits any of these arguments and has lead to a
more stable driver, even with HW where the bandwidth calculations
have become more and more complex.

We do want this code to be kosher for the kernel but beyond that
we want to refactor this code as little as possible for the
above-stated reasons.

Note the "NOTE" at the top of the file regarding this.

As for the specifics of this patch, I'll leave that to Siqueira,
since he's been a lot deeper into this code than I have.

Harry

> Signed-off-by: Rafael Mendonca <rafaelmendsr@xxxxxxxxx>
> ---
> .../dc/dml/dcn314/display_mode_vba_314.c | 106 +-----------------
> .../dc/dml/dcn314/display_rq_dlg_calc_314.c | 91 +--------------
> 2 files changed, 6 insertions(+), 191 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c b/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c
> index 0d12fd079cd6..6e43cd21a7d3 100644
> --- a/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c
> +++ b/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c
> @@ -32,6 +32,7 @@
> #include "../display_mode_lib.h"
> #include "display_mode_vba_314.h"
> #include "../dml_inline_defs.h"
> +#include "dml/dcn30/display_mode_vba_30.h"
>
> /*
> * NOTE:
> @@ -90,17 +91,6 @@ typedef struct {
> #define BPP_INVALID 0
> #define BPP_BLENDED_PIPE 0xffffffff
>
> -static bool CalculateBytePerPixelAnd256BBlockSizes(
> - enum source_format_class SourcePixelFormat,
> - enum dm_swizzle_mode SurfaceTiling,
> - unsigned int *BytePerPixelY,
> - unsigned int *BytePerPixelC,
> - double *BytePerPixelDETY,
> - double *BytePerPixelDETC,
> - unsigned int *BlockHeight256BytesY,
> - unsigned int *BlockHeight256BytesC,
> - unsigned int *BlockWidth256BytesY,
> - unsigned int *BlockWidth256BytesC);
> static void DisplayPipeConfiguration(struct display_mode_lib *mode_lib);
> static void DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation(struct display_mode_lib *mode_lib);
> static unsigned int dscceComputeDelay(
> @@ -2178,7 +2168,7 @@ static void DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerforman
> DTRACE(" return_bus_bw = %f", v->ReturnBW);
>
> for (k = 0; k < v->NumberOfActivePlanes; ++k) {
> - CalculateBytePerPixelAnd256BBlockSizes(
> + dml30_CalculateBytePerPixelAnd256BBlockSizes(
> v->SourcePixelFormat[k],
> v->SurfaceTiling[k],
> &v->BytePerPixelY[k],
> @@ -3317,7 +3307,7 @@ static void DisplayPipeConfiguration(struct display_mode_lib *mode_lib)
>
> for (k = 0; k < v->NumberOfActivePlanes; ++k) {
>
> - CalculateBytePerPixelAnd256BBlockSizes(
> + dml30_CalculateBytePerPixelAnd256BBlockSizes(
> v->SourcePixelFormat[k],
> v->SurfaceTiling[k],
> &BytePerPixY[k],
> @@ -3371,94 +3361,6 @@ static void DisplayPipeConfiguration(struct display_mode_lib *mode_lib)
> &dummysinglestring);
> }
>
> -static bool CalculateBytePerPixelAnd256BBlockSizes(
> - enum source_format_class SourcePixelFormat,
> - enum dm_swizzle_mode SurfaceTiling,
> - unsigned int *BytePerPixelY,
> - unsigned int *BytePerPixelC,
> - double *BytePerPixelDETY,
> - double *BytePerPixelDETC,
> - unsigned int *BlockHeight256BytesY,
> - unsigned int *BlockHeight256BytesC,
> - unsigned int *BlockWidth256BytesY,
> - unsigned int *BlockWidth256BytesC)
> -{
> - if (SourcePixelFormat == dm_444_64) {
> - *BytePerPixelDETY = 8;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 8;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_444_32 || SourcePixelFormat == dm_rgbe) {
> - *BytePerPixelDETY = 4;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 4;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_444_16) {
> - *BytePerPixelDETY = 2;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 2;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_444_8) {
> - *BytePerPixelDETY = 1;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 1;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_rgbe_alpha) {
> - *BytePerPixelDETY = 4;
> - *BytePerPixelDETC = 1;
> - *BytePerPixelY = 4;
> - *BytePerPixelC = 1;
> - } else if (SourcePixelFormat == dm_420_8) {
> - *BytePerPixelDETY = 1;
> - *BytePerPixelDETC = 2;
> - *BytePerPixelY = 1;
> - *BytePerPixelC = 2;
> - } else if (SourcePixelFormat == dm_420_12) {
> - *BytePerPixelDETY = 2;
> - *BytePerPixelDETC = 4;
> - *BytePerPixelY = 2;
> - *BytePerPixelC = 4;
> - } else {
> - *BytePerPixelDETY = 4.0 / 3;
> - *BytePerPixelDETC = 8.0 / 3;
> - *BytePerPixelY = 2;
> - *BytePerPixelC = 4;
> - }
> -
> - if ((SourcePixelFormat == dm_444_64 || SourcePixelFormat == dm_444_32 || SourcePixelFormat == dm_444_16 || SourcePixelFormat == dm_444_8 || SourcePixelFormat == dm_mono_16
> - || SourcePixelFormat == dm_mono_8 || SourcePixelFormat == dm_rgbe)) {
> - if (SurfaceTiling == dm_sw_linear) {
> - *BlockHeight256BytesY = 1;
> - } else if (SourcePixelFormat == dm_444_64) {
> - *BlockHeight256BytesY = 4;
> - } else if (SourcePixelFormat == dm_444_8) {
> - *BlockHeight256BytesY = 16;
> - } else {
> - *BlockHeight256BytesY = 8;
> - }
> - *BlockWidth256BytesY = 256U / *BytePerPixelY / *BlockHeight256BytesY;
> - *BlockHeight256BytesC = 0;
> - *BlockWidth256BytesC = 0;
> - } else {
> - if (SurfaceTiling == dm_sw_linear) {
> - *BlockHeight256BytesY = 1;
> - *BlockHeight256BytesC = 1;
> - } else if (SourcePixelFormat == dm_rgbe_alpha) {
> - *BlockHeight256BytesY = 8;
> - *BlockHeight256BytesC = 16;
> - } else if (SourcePixelFormat == dm_420_8) {
> - *BlockHeight256BytesY = 16;
> - *BlockHeight256BytesC = 8;
> - } else {
> - *BlockHeight256BytesY = 8;
> - *BlockHeight256BytesC = 8;
> - }
> - *BlockWidth256BytesY = 256U / *BytePerPixelY / *BlockHeight256BytesY;
> - *BlockWidth256BytesC = 256U / *BytePerPixelC / *BlockHeight256BytesC;
> - }
> - return true;
> -}
> -
> static double CalculateTWait(unsigned int PrefetchMode, double DRAMClockChangeLatency, double UrgentLatency, double SREnterPlusExitTime)
> {
> if (PrefetchMode == 0) {
> @@ -3948,7 +3850,7 @@ void dml314_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_
> /*Bandwidth Support Check*/
>
> for (k = 0; k < v->NumberOfActivePlanes; k++) {
> - CalculateBytePerPixelAnd256BBlockSizes(
> + dml30_CalculateBytePerPixelAnd256BBlockSizes(
> v->SourcePixelFormat[k],
> v->SurfaceTiling[k],
> &v->BytePerPixelY[k],
> diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_rq_dlg_calc_314.c b/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_rq_dlg_calc_314.c
> index 61ee9ba063a7..a373d35dd473 100644
> --- a/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_rq_dlg_calc_314.c
> +++ b/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_rq_dlg_calc_314.c
> @@ -28,94 +28,7 @@
> #include "../display_mode_vba.h"
> #include "../dml_inline_defs.h"
> #include "display_rq_dlg_calc_314.h"
> -
> -static bool CalculateBytePerPixelAnd256BBlockSizes(
> - enum source_format_class SourcePixelFormat,
> - enum dm_swizzle_mode SurfaceTiling,
> - unsigned int *BytePerPixelY,
> - unsigned int *BytePerPixelC,
> - double *BytePerPixelDETY,
> - double *BytePerPixelDETC,
> - unsigned int *BlockHeight256BytesY,
> - unsigned int *BlockHeight256BytesC,
> - unsigned int *BlockWidth256BytesY,
> - unsigned int *BlockWidth256BytesC)
> -{
> - if (SourcePixelFormat == dm_444_64) {
> - *BytePerPixelDETY = 8;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 8;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_444_32 || SourcePixelFormat == dm_rgbe) {
> - *BytePerPixelDETY = 4;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 4;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_444_16 || SourcePixelFormat == dm_444_16) {
> - *BytePerPixelDETY = 2;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 2;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_444_8) {
> - *BytePerPixelDETY = 1;
> - *BytePerPixelDETC = 0;
> - *BytePerPixelY = 1;
> - *BytePerPixelC = 0;
> - } else if (SourcePixelFormat == dm_rgbe_alpha) {
> - *BytePerPixelDETY = 4;
> - *BytePerPixelDETC = 1;
> - *BytePerPixelY = 4;
> - *BytePerPixelC = 1;
> - } else if (SourcePixelFormat == dm_420_8) {
> - *BytePerPixelDETY = 1;
> - *BytePerPixelDETC = 2;
> - *BytePerPixelY = 1;
> - *BytePerPixelC = 2;
> - } else if (SourcePixelFormat == dm_420_12) {
> - *BytePerPixelDETY = 2;
> - *BytePerPixelDETC = 4;
> - *BytePerPixelY = 2;
> - *BytePerPixelC = 4;
> - } else {
> - *BytePerPixelDETY = 4.0 / 3;
> - *BytePerPixelDETC = 8.0 / 3;
> - *BytePerPixelY = 2;
> - *BytePerPixelC = 4;
> - }
> -
> - if ((SourcePixelFormat == dm_444_64 || SourcePixelFormat == dm_444_32 || SourcePixelFormat == dm_444_16 || SourcePixelFormat == dm_444_8 || SourcePixelFormat == dm_mono_16
> - || SourcePixelFormat == dm_mono_8 || SourcePixelFormat == dm_rgbe)) {
> - if (SurfaceTiling == dm_sw_linear)
> - *BlockHeight256BytesY = 1;
> - else if (SourcePixelFormat == dm_444_64)
> - *BlockHeight256BytesY = 4;
> - else if (SourcePixelFormat == dm_444_8)
> - *BlockHeight256BytesY = 16;
> - else
> - *BlockHeight256BytesY = 8;
> -
> - *BlockWidth256BytesY = 256U / *BytePerPixelY / *BlockHeight256BytesY;
> - *BlockHeight256BytesC = 0;
> - *BlockWidth256BytesC = 0;
> - } else {
> - if (SurfaceTiling == dm_sw_linear) {
> - *BlockHeight256BytesY = 1;
> - *BlockHeight256BytesC = 1;
> - } else if (SourcePixelFormat == dm_rgbe_alpha) {
> - *BlockHeight256BytesY = 8;
> - *BlockHeight256BytesC = 16;
> - } else if (SourcePixelFormat == dm_420_8) {
> - *BlockHeight256BytesY = 16;
> - *BlockHeight256BytesC = 8;
> - } else {
> - *BlockHeight256BytesY = 8;
> - *BlockHeight256BytesC = 8;
> - }
> - *BlockWidth256BytesY = 256U / *BytePerPixelY / *BlockHeight256BytesY;
> - *BlockWidth256BytesC = 256U / *BytePerPixelC / *BlockHeight256BytesC;
> - }
> - return true;
> -}
> +#include "dml/dcn30/display_mode_vba_30.h"
>
> static bool is_dual_plane(enum source_format_class source_format)
> {
> @@ -468,7 +381,7 @@ static void get_meta_and_pte_attr(
> double byte_per_pixel_det_y;
> double byte_per_pixel_det_c;
>
> - CalculateBytePerPixelAnd256BBlockSizes(
> + dml30_CalculateBytePerPixelAnd256BBlockSizes(
> (enum source_format_class) (source_format),
> (enum dm_swizzle_mode) (tiling),
> &bytes_per_element_y,

Next message: Nícolas F. R. A. Prado: "Re: [PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func"
Previous message: Sean Christopherson: "Re: [PATCH] KVM: x86/mmu: Release the pfn in handle_abnormal_pfn() error path"
In reply to: Rafael Mendonca: "[PATCH] drm/amd/display: Remove duplicate code for DCN314 DML calculation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]