[PATCH 0/3] Fix some DC regressions on DCE 6-8

[PATCH 0/3] Fix some DC regressions on DCE 6-8

[Timur Kristóf]

This series fixes some regressions in DC, mainly on DCE6-8.

There is a fix for a recent regression caused by refactoring
the DSC cap calculation that affects DCE8-10,  as well as
some fixes related to clock sources in DCE 6.

Timur Kristóf (3):
  drm/amd/display: Fix refactored DSC cap calculation
  drm/amd/display: Don't overwrite dce60_clk_mgr
  drm/amd/display: Fix DCE 6.0 and 6.4 PLL programming.

 .../gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c  |  1 -
 .../display/dc/clk_mgr/dce100/dce_clk_mgr.c   |  5 +++
 drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c   | 17 +++++-----
 .../dc/resource/dce60/dce60_resource.c        | 34 +++++++++++--------
 4 files changed, 34 insertions(+), 23 deletions(-)

-- 
2.50.1

[PATCH 1/3] drm/amd/display: Fix refactored DSC cap calculation

[Timur Kristóf]

After refactoring the DSC capability calculation, the
get_min_slice_count_for_odm could crash on some GPUs due to a
division by zero when max_total_throughput_mps was zero.
As a result, DC was broken when connecting a GPU that doesn't
support DSC to a monitor that supports DSC.
Tested on Oland (DCE 6) and Fiji (DCE 10).

This commit fixes it by returning zero instead.

Fixes: 4909b8b3846c ("drm/amd/display: Refactor DSC cap calculations")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
index a454d16e6586..4169ece9c535 100644
--- a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
+++ b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
@@ -642,14 +642,15 @@ static unsigned int get_min_slice_count_for_odm(
 	unsigned int max_dispclk_khz;
 
 	/* get max pixel rate and combine caps */
-	max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps * 1000;
-	if (dsc && dsc->ctx->dc) {
-		if (dsc->ctx->dc->clk_mgr &&
-			dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz) {
-			/* dispclk is available */
-			max_dispclk_khz = dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(dsc->ctx->dc->clk_mgr, CLK_TYPE_DISPCLK);
-		}
-	}
+	if (dsc && dsc->ctx->dc && dsc->ctx->dc->clk_mgr &&
+		dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz)
+		max_dispclk_khz =
+			dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(
+				dsc->ctx->dc->clk_mgr, CLK_TYPE_DISPCLK);
+	else if (dsc_enc_caps->max_total_throughput_mps)
+		max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps * 1000;
+	else
+		return 0;
 
 	/* consider minimum odm slices required due to
 	 * 1) display pipe throughput (dispclk)
-- 
2.50.1

[PATCH 2/3] drm/amd/display: Don't overwrite dce60_clk_mgr

[Timur Kristóf]

dc_clk_mgr_create accidentally overwrites the dce60_clk_mgr
with the dce_clk_mgr, causing incorrect behaviour on DCE6.
Fix it by removing the extra dce_clk_mgr_construct.

Fixes: 62eab49faae7 ("drm/amd/display: hide VGH asic specific structs")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
index 33b9d36619ff..4071851f9e86 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
@@ -158,7 +158,6 @@ struct clk_mgr *dc_clk_mgr_create(struct dc_context *ctx, struct pp_smu_funcs *p
 			return NULL;
 		}
 		dce60_clk_mgr_construct(ctx, clk_mgr);
-		dce_clk_mgr_construct(ctx, clk_mgr);
 		return &clk_mgr->base;
 	}
 #endif
-- 
2.50.1

[PATCH 3/3] drm/amd/display: Fix DCE 6.0 and 6.4 PLL programming.

[Timur Kristóf]

Apparently, both DCE 6.0 and 6.4 have 3 PLLs, but PLL0 can only
be used for DP. Make sure to initialize the correct amount of PLLs
in DC for these DCE versions and use PLL0 only for DP.

Also, on DCE 6.0 and 6.4, the PLL0 needs to be powered on at
initialization as opposed to DCE 6.1 and 7.x which use a different
clock source for DFS.

The following functions were used as reference from the	old
radeon driver implementation of	DCE 6.x:
- radeon_atom_pick_pll
- atombios_crtc_set_disp_eng_pll

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 .../display/dc/clk_mgr/dce100/dce_clk_mgr.c   |  5 +++
 .../dc/resource/dce60/dce60_resource.c        | 34 +++++++++++--------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
index 26feefbb8990..f5ad0a177038 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
@@ -245,6 +245,11 @@ int dce_set_clock(
 	pxl_clk_params.target_pixel_clock_100hz = requested_clk_khz * 10;
 	pxl_clk_params.pll_id = CLOCK_SOURCE_ID_DFS;
 
+	/* DCE 6.0, DCE 6.4: engine clock is the same as PLL0 */
+	if (clk_mgr_base->ctx->dce_version == DCE_VERSION_6_0 ||
+	    clk_mgr_base->ctx->dce_version == DCE_VERSION_6_4)
+		pxl_clk_params.pll_id = CLOCK_SOURCE_ID_PLL0;
+
 	if (clk_mgr_dce->dfs_bypass_active)
 		pxl_clk_params.flags.SET_DISPCLK_DFS_BYPASS = true;
 
diff --git a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
index 58b59d52dc9d..53b60044653f 100644
--- a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
@@ -373,7 +373,7 @@ static const struct resource_caps res_cap = {
 		.num_timing_generator = 6,
 		.num_audio = 6,
 		.num_stream_encoder = 6,
-		.num_pll = 2,
+		.num_pll = 3,
 		.num_ddc = 6,
 };
 
@@ -389,7 +389,7 @@ static const struct resource_caps res_cap_64 = {
 		.num_timing_generator = 2,
 		.num_audio = 2,
 		.num_stream_encoder = 2,
-		.num_pll = 2,
+		.num_pll = 3,
 		.num_ddc = 2,
 };
 
@@ -973,21 +973,24 @@ static bool dce60_construct(
 
 	if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
 		pool->base.dp_clock_source =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
 
+		/* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
 		pool->base.clock_sources[0] =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], false);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
 		pool->base.clock_sources[1] =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
 		pool->base.clk_src_count = 2;
 
 	} else {
 		pool->base.dp_clock_source =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
 
 		pool->base.clock_sources[0] =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
-		pool->base.clk_src_count = 1;
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
+		pool->base.clock_sources[1] =
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
+		pool->base.clk_src_count = 2;
 	}
 
 	if (pool->base.dp_clock_source == NULL) {
@@ -1365,21 +1368,24 @@ static bool dce64_construct(
 
 	if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
 		pool->base.dp_clock_source =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
 
+		/* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
 		pool->base.clock_sources[0] =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], false);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
 		pool->base.clock_sources[1] =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
 		pool->base.clk_src_count = 2;
 
 	} else {
 		pool->base.dp_clock_source =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], true);
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
 
 		pool->base.clock_sources[0] =
-				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
-		pool->base.clk_src_count = 1;
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
+		pool->base.clock_sources[1] =
+			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
+		pool->base.clk_src_count = 2;
 	}
 
 	if (pool->base.dp_clock_source == NULL) {
-- 
2.50.1

Re: [PATCH 1/3] drm/amd/display: Fix refactored DSC cap calculation

[Rodrigo Siqueira]

On 010/22, Timur Kristóf wrote:
> After refactoring the DSC capability calculation, the
> get_min_slice_count_for_odm could crash on some GPUs due to a
> division by zero when max_total_throughput_mps was zero.
> As a result, DC was broken when connecting a GPU that doesn't
> support DSC to a monitor that supports DSC.
> Tested on Oland (DCE 6) and Fiji (DCE 10).
> 
> This commit fixes it by returning zero instead.
> 
> Fixes: 4909b8b3846c ("drm/amd/display: Refactor DSC cap calculations")
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>  drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> index a454d16e6586..4169ece9c535 100644
> --- a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> +++ b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> @@ -642,14 +642,15 @@ static unsigned int get_min_slice_count_for_odm(
>  	unsigned int max_dispclk_khz;
>  
>  	/* get max pixel rate and combine caps */
> -	max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps * 1000;
> -	if (dsc && dsc->ctx->dc) {
> -		if (dsc->ctx->dc->clk_mgr &&
> -			dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz) {
> -			/* dispclk is available */
> -			max_dispclk_khz = dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(dsc->ctx->dc->clk_mgr, CLK_TYPE_DISPCLK);
> -		}
> -	}
> +	if (dsc && dsc->ctx->dc && dsc->ctx->dc->clk_mgr &&
> +		dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz)
> +		max_dispclk_khz =
> +			dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(
> +				dsc->ctx->dc->clk_mgr, CLK_TYPE_DISPCLK);
> +	else if (dsc_enc_caps->max_total_throughput_mps)
> +		max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps * 1000;
> +	else
> +		return 0;
>  
>  	/* consider minimum odm slices required due to
>  	 * 1) display pipe throughput (dispclk)
> -- 
> 2.50.1
>

This patch lgtm.

Reviewed-by: Rodrigo Siqueira <siqueira@igalia.com>

I added other display folks to this patch.

I'm not sure if the original modification (4909b8b3846c) is already in
the stable kernel; if so, it could be a good idea to send this fix to
the stable kernel as well.

Thanks

-- 
Rodrigo Siqueira

Re: [PATCH 2/3] drm/amd/display: Don't overwrite dce60_clk_mgr

[Rodrigo Siqueira]

On 07/22, Timur Kristóf wrote:
> dc_clk_mgr_create accidentally overwrites the dce60_clk_mgr
> with the dce_clk_mgr, causing incorrect behaviour on DCE6.

Could you ellaborate on what do you mean by incorrect behaviour?

> Fix it by removing the extra dce_clk_mgr_construct.
> 
> Fixes: 62eab49faae7 ("drm/amd/display: hide VGH asic specific structs")
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>  drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> index 33b9d36619ff..4071851f9e86 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> @@ -158,7 +158,6 @@ struct clk_mgr *dc_clk_mgr_create(struct dc_context *ctx, struct pp_smu_funcs *p
>  			return NULL;
>  		}
>  		dce60_clk_mgr_construct(ctx, clk_mgr);
> -		dce_clk_mgr_construct(ctx, clk_mgr);

I suppose this error was not detected at the time because of the
CONFIG_DRM_AMD_DC_SI guard. This lgtm, but I guess it would be nice to
test this patch with other SI devices just to be sure

Reviewed-by: Rodrigo Siqueira <siqueira@igalia.com>

I added other display folks.

Thanks

>  		return &clk_mgr->base;
>  	}
>  #endif
> -- 
> 2.50.1
> 

-- 
Rodrigo Siqueira

Re: [PATCH 3/3] drm/amd/display: Fix DCE 6.0 and 6.4 PLL programming.

[Rodrigo Siqueira]

On 07/22, Timur Kristóf wrote:
> Apparently, both DCE 6.0 and 6.4 have 3 PLLs, but PLL0 can only
> be used for DP. Make sure to initialize the correct amount of PLLs
> in DC for these DCE versions and use PLL0 only for DP.
> 
> Also, on DCE 6.0 and 6.4, the PLL0 needs to be powered on at
> initialization as opposed to DCE 6.1 and 7.x which use a different
> clock source for DFS.
> 
> The following functions were used as reference from the	old
> radeon driver implementation of	DCE 6.x:
> - radeon_atom_pick_pll
> - atombios_crtc_set_disp_eng_pll
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>  .../display/dc/clk_mgr/dce100/dce_clk_mgr.c   |  5 +++
>  .../dc/resource/dce60/dce60_resource.c        | 34 +++++++++++--------
>  2 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> index 26feefbb8990..f5ad0a177038 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> @@ -245,6 +245,11 @@ int dce_set_clock(
>  	pxl_clk_params.target_pixel_clock_100hz = requested_clk_khz * 10;
>  	pxl_clk_params.pll_id = CLOCK_SOURCE_ID_DFS;
>  
> +	/* DCE 6.0, DCE 6.4: engine clock is the same as PLL0 */
> +	if (clk_mgr_base->ctx->dce_version == DCE_VERSION_6_0 ||
> +	    clk_mgr_base->ctx->dce_version == DCE_VERSION_6_4)
> +		pxl_clk_params.pll_id = CLOCK_SOURCE_ID_PLL0;
> +
>  	if (clk_mgr_dce->dfs_bypass_active)
>  		pxl_clk_params.flags.SET_DISPCLK_DFS_BYPASS = true;
>  
> diff --git a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> index 58b59d52dc9d..53b60044653f 100644
> --- a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> @@ -373,7 +373,7 @@ static const struct resource_caps res_cap = {
>  		.num_timing_generator = 6,
>  		.num_audio = 6,
>  		.num_stream_encoder = 6,
> -		.num_pll = 2,
> +		.num_pll = 3,
>  		.num_ddc = 6,
>  };
>  
> @@ -389,7 +389,7 @@ static const struct resource_caps res_cap_64 = {
>  		.num_timing_generator = 2,
>  		.num_audio = 2,
>  		.num_stream_encoder = 2,
> -		.num_pll = 2,
> +		.num_pll = 3,
>  		.num_ddc = 2,
>  };
>  
> @@ -973,21 +973,24 @@ static bool dce60_construct(
>  
>  	if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
>  		pool->base.dp_clock_source =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
>  
> +		/* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
>  		pool->base.clock_sources[0] =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], false);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
>  		pool->base.clock_sources[1] =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
>  		pool->base.clk_src_count = 2;
>  
>  	} else {
>  		pool->base.dp_clock_source =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
>  
>  		pool->base.clock_sources[0] =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> -		pool->base.clk_src_count = 1;
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +		pool->base.clock_sources[1] =
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> +		pool->base.clk_src_count = 2;
>  	}
>  
>  	if (pool->base.dp_clock_source == NULL) {
> @@ -1365,21 +1368,24 @@ static bool dce64_construct(
>  
>  	if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
>  		pool->base.dp_clock_source =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
>  
> +		/* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
>  		pool->base.clock_sources[0] =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], false);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
>  		pool->base.clock_sources[1] =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
>  		pool->base.clk_src_count = 2;
>  
>  	} else {
>  		pool->base.dp_clock_source =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], true);
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
>  
>  		pool->base.clock_sources[0] =
> -				dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> -		pool->base.clk_src_count = 1;
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +		pool->base.clock_sources[1] =
> +			dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> +		pool->base.clk_src_count = 2;
>  	}
>  
>  	if (pool->base.dp_clock_source == NULL) {
> -- 
> 2.50.1
>

Reviewed-by: Rodrigo Siqueira <siqueira@igalia.com>

Again, I'm adding other displays folks for further review.

Thanks 

-- 
Rodrigo Siqueira

Re: [PATCH 0/3] Fix some DC regressions on DCE 6-8

[Rodrigo Siqueira]

On 07/22, Timur Kristóf wrote:
> This series fixes some regressions in DC, mainly on DCE6-8.
> 
> There is a fix for a recent regression caused by refactoring
> the DSC cap calculation that affects DCE8-10,  as well as
> some fixes related to clock sources in DCE 6.
> 
> Timur Kristóf (3):
>   drm/amd/display: Fix refactored DSC cap calculation
>   drm/amd/display: Don't overwrite dce60_clk_mgr
>   drm/amd/display: Fix DCE 6.0 and 6.4 PLL programming.
> 
>  .../gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c  |  1 -
>  .../display/dc/clk_mgr/dce100/dce_clk_mgr.c   |  5 +++
>  drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c   | 17 +++++-----
>  .../dc/resource/dce60/dce60_resource.c        | 34 +++++++++++--------
>  4 files changed, 34 insertions(+), 23 deletions(-)
> 
> -- 
> 2.50.1
>

Hi Dan,

I think this is a good candidate for next week's promotion. Tbh, I doubt
you will see any issues in the ASICs that you are currently using for
test. Perhaps it would be nice to have some old DCE devices for the next
round of testing that includes this series?

Timur also has the following series that could be put together with this
one in next week's promotion to save time. All of his patches are
focused on DCE devices (but the series below could have some impact on
new devices), and the last one addresses analog connector.

https://patchwork.freedesktop.org/series/152016/

Thanks

-- 
Rodrigo Siqueira

Re: [PATCH 2/3] drm/amd/display: Don't overwrite dce60_clk_mgr

[Timur Kristóf]

On Thu, 2025-07-24 at 16:40 -0600, Rodrigo Siqueira wrote:
> On 07/22, Timur Kristóf wrote:
> > dc_clk_mgr_create accidentally overwrites the dce60_clk_mgr
> > with the dce_clk_mgr, causing incorrect behaviour on DCE6.
> 
> Could you ellaborate on what do you mean by incorrect behaviour?

By incorrect behaviour, I mean that the code was using dce_clk_mgr when
it should have used the dce60_clk_mgr. So the DCE 6 specific functions
were not correctly hooked up.

dce60_clk_mgr was added in 3ecb3b794e2c and according to that commit,
the main difference is that DCE 6 doesn't have the DPREFCLK_CNTL
registers.

> 
> > Fix it by removing the extra dce_clk_mgr_construct.
> > 
> > Fixes: 62eab49faae7 ("drm/amd/display: hide VGH asic specific
> > structs")
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >  drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> > b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> > index 33b9d36619ff..4071851f9e86 100644
> > --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> > +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> > @@ -158,7 +158,6 @@ struct clk_mgr *dc_clk_mgr_create(struct
> > dc_context *ctx, struct pp_smu_funcs *p
> >  			return NULL;
> >  		}
> >  		dce60_clk_mgr_construct(ctx, clk_mgr);
> > -		dce_clk_mgr_construct(ctx, clk_mgr);
> 
> I suppose this error was not detected at the time because of the
> CONFIG_DRM_AMD_DC_SI guard. This lgtm, but I guess it would be nice
> to
> test this patch with other SI devices just to be sure

This series just contains some obvious regression fixes that I felt
were the most important and easiest to review.

SI definitely needs more work. I have a work in progress branch for
this; I'll send the patches in a separate series. (The main things that
are currently missing are fixing the display clock and taking
pm_display_cfg into use in legacy_dpm and si_dpm to include DC display
requirements in power management decisions.)

> 
> Reviewed-by: Rodrigo Siqueira <siqueira@igalia.com>
> 
> I added other display folks.
> 
> Thanks
> 
> >  		return &clk_mgr->base;
> >  	}
> >  #endif
> > -- 
> > 2.50.1
> > 

Re: [PATCH 1/3] drm/amd/display: Fix refactored DSC cap calculation

[Alex Hung]

This patch may be related and conflict to 
https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg125873.html

amd-staging-drm-next should include the above patch but it is not 
updated for 2 weeks, so let's wait for ASDN to be updated.


On 7/24/25 16:32, Rodrigo Siqueira wrote:
> On 010/22, Timur Kristóf wrote:
>> After refactoring the DSC capability calculation, the
>> get_min_slice_count_for_odm could crash on some GPUs due to a
>> division by zero when max_total_throughput_mps was zero.
>> As a result, DC was broken when connecting a GPU that doesn't
>> support DSC to a monitor that supports DSC.
>> Tested on Oland (DCE 6) and Fiji (DCE 10).
>>
>> This commit fixes it by returning zero instead.
>>
>> Fixes: 4909b8b3846c ("drm/amd/display: Refactor DSC cap calculations")
>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
>> ---
>>   drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
>> index a454d16e6586..4169ece9c535 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
>> @@ -642,14 +642,15 @@ static unsigned int get_min_slice_count_for_odm(
>>   	unsigned int max_dispclk_khz;
>>   
>>   	/* get max pixel rate and combine caps */
>> -	max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps * 1000;
>> -	if (dsc && dsc->ctx->dc) {
>> -		if (dsc->ctx->dc->clk_mgr &&
>> -			dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz) {
>> -			/* dispclk is available */
>> -			max_dispclk_khz = dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(dsc->ctx->dc->clk_mgr, CLK_TYPE_DISPCLK);
>> -		}
>> -	}
>> +	if (dsc && dsc->ctx->dc && dsc->ctx->dc->clk_mgr &&
>> +		dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz)
>> +		max_dispclk_khz =
>> +			dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(
>> +				dsc->ctx->dc->clk_mgr, CLK_TYPE_DISPCLK);
>> +	else if (dsc_enc_caps->max_total_throughput_mps)
>> +		max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps * 1000;
>> +	else
>> +		return 0;
>>   
>>   	/* consider minimum odm slices required due to
>>   	 * 1) display pipe throughput (dispclk)
>> -- 
>> 2.50.1
>>
> 
> This patch lgtm.
> 
> Reviewed-by: Rodrigo Siqueira <siqueira@igalia.com>
> 
> I added other display folks to this patch.
> 
> I'm not sure if the original modification (4909b8b3846c) is already in
> the stable kernel; if so, it could be a good idea to send this fix to
> the stable kernel as well.
> 
> Thanks
> 

Re: [PATCH 1/3] drm/amd/display: Fix refactored DSC cap calculation

[Timur Kristóf]

[-- Attachment #1: Type: text/plain, Size: 3150 bytes --]

Hi,

Alex Hung <alex.hung@amd.com> ezt írta (időpont: 2025. júl. 25., Pén 22:26):

> This patch may be related and conflict to
> https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg125873.html
>
> amd-staging-drm-next should include the above patch but it is not
> updated for 2 weeks, so let's wait for ASDN to be updated.
>

Yes, it indeed looks like that patch addresses the same problem.



>
> On 7/24/25 16:32, Rodrigo Siqueira wrote:
> > On 010/22, Timur Kristóf wrote:
> >> After refactoring the DSC capability calculation, the
> >> get_min_slice_count_for_odm could crash on some GPUs due to a
> >> division by zero when max_total_throughput_mps was zero.
> >> As a result, DC was broken when connecting a GPU that doesn't
> >> support DSC to a monitor that supports DSC.
> >> Tested on Oland (DCE 6) and Fiji (DCE 10).
> >>
> >> This commit fixes it by returning zero instead.
> >>
> >> Fixes: 4909b8b3846c ("drm/amd/display: Refactor DSC cap calculations")
> >> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> >> ---
> >>   drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c | 17 +++++++++--------
> >>   1 file changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> >> index a454d16e6586..4169ece9c535 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> >> @@ -642,14 +642,15 @@ static unsigned int get_min_slice_count_for_odm(
> >>      unsigned int max_dispclk_khz;
> >>
> >>      /* get max pixel rate and combine caps */
> >> -    max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps * 1000;
> >> -    if (dsc && dsc->ctx->dc) {
> >> -            if (dsc->ctx->dc->clk_mgr &&
> >> -                    dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz) {
> >> -                    /* dispclk is available */
> >> -                    max_dispclk_khz =
> dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(dsc->ctx->dc->clk_mgr,
> CLK_TYPE_DISPCLK);
> >> -            }
> >> -    }
> >> +    if (dsc && dsc->ctx->dc && dsc->ctx->dc->clk_mgr &&
> >> +            dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz)
> >> +            max_dispclk_khz =
> >> +                    dsc->ctx->dc->clk_mgr->funcs->get_max_clock_khz(
> >> +                            dsc->ctx->dc->clk_mgr, CLK_TYPE_DISPCLK);
> >> +    else if (dsc_enc_caps->max_total_throughput_mps)
> >> +            max_dispclk_khz = dsc_enc_caps->max_total_throughput_mps *
> 1000;
> >> +    else
> >> +            return 0;
> >>
> >>      /* consider minimum odm slices required due to
> >>       * 1) display pipe throughput (dispclk)
> >> --
> >> 2.50.1
> >>
> >
> > This patch lgtm.
> >
> > Reviewed-by: Rodrigo Siqueira <siqueira@igalia.com>
> >
> > I added other display folks to this patch.
> >
> > I'm not sure if the original modification (4909b8b3846c) is already in
> > the stable kernel; if so, it could be a good idea to send this fix to
> > the stable kernel as well.
> >
> > Thanks
> >
>
>

[-- Attachment #2: Type: text/html, Size: 4780 bytes --]

Re: [PATCH 3/3] drm/amd/display: Fix DCE 6.0 and 6.4 PLL programming.

[Alex Deucher]

On Tue, Jul 22, 2025 at 12:23 PM Timur Kristóf <timur.kristof@gmail.com> wrote:
>
> Apparently, both DCE 6.0 and 6.4 have 3 PLLs, but PLL0 can only
> be used for DP. Make sure to initialize the correct amount of PLLs
> in DC for these DCE versions and use PLL0 only for DP.
>
> Also, on DCE 6.0 and 6.4, the PLL0 needs to be powered on at
> initialization as opposed to DCE 6.1 and 7.x which use a different
> clock source for DFS.
>
> The following functions were used as reference from the old
> radeon driver implementation of DCE 6.x:
> - radeon_atom_pick_pll
> - atombios_crtc_set_disp_eng_pll
>
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  .../display/dc/clk_mgr/dce100/dce_clk_mgr.c   |  5 +++
>  .../dc/resource/dce60/dce60_resource.c        | 34 +++++++++++--------
>  2 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> index 26feefbb8990..f5ad0a177038 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> @@ -245,6 +245,11 @@ int dce_set_clock(
>         pxl_clk_params.target_pixel_clock_100hz = requested_clk_khz * 10;
>         pxl_clk_params.pll_id = CLOCK_SOURCE_ID_DFS;
>
> +       /* DCE 6.0, DCE 6.4: engine clock is the same as PLL0 */
> +       if (clk_mgr_base->ctx->dce_version == DCE_VERSION_6_0 ||
> +           clk_mgr_base->ctx->dce_version == DCE_VERSION_6_4)
> +               pxl_clk_params.pll_id = CLOCK_SOURCE_ID_PLL0;
> +
>         if (clk_mgr_dce->dfs_bypass_active)
>                 pxl_clk_params.flags.SET_DISPCLK_DFS_BYPASS = true;
>
> diff --git a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> index 58b59d52dc9d..53b60044653f 100644
> --- a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> @@ -373,7 +373,7 @@ static const struct resource_caps res_cap = {
>                 .num_timing_generator = 6,
>                 .num_audio = 6,
>                 .num_stream_encoder = 6,
> -               .num_pll = 2,
> +               .num_pll = 3,
>                 .num_ddc = 6,
>  };
>
> @@ -389,7 +389,7 @@ static const struct resource_caps res_cap_64 = {
>                 .num_timing_generator = 2,
>                 .num_audio = 2,
>                 .num_stream_encoder = 2,
> -               .num_pll = 2,
> +               .num_pll = 3,
>                 .num_ddc = 2,
>  };
>
> @@ -973,21 +973,24 @@ static bool dce60_construct(
>
>         if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
>
> +               /* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
>                 pool->base.clock_sources[1] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
>                 pool->base.clk_src_count = 2;
>
>         } else {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
>
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> -               pool->base.clk_src_count = 1;
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +               pool->base.clock_sources[1] =
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> +               pool->base.clk_src_count = 2;
>         }
>
>         if (pool->base.dp_clock_source == NULL) {
> @@ -1365,21 +1368,24 @@ static bool dce64_construct(
>
>         if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
>
> +               /* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
>                 pool->base.clock_sources[1] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
>                 pool->base.clk_src_count = 2;
>
>         } else {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
>
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> -               pool->base.clk_src_count = 1;
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +               pool->base.clock_sources[1] =
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> +               pool->base.clk_src_count = 2;
>         }
>
>         if (pool->base.dp_clock_source == NULL) {
> --
> 2.50.1
>

Re: [PATCH 2/3] drm/amd/display: Don't overwrite dce60_clk_mgr

[Alex Deucher]

On Tue, Jul 22, 2025 at 11:58 AM Timur Kristóf <timur.kristof@gmail.com> wrote:
>
> dc_clk_mgr_create accidentally overwrites the dce60_clk_mgr
> with the dce_clk_mgr, causing incorrect behaviour on DCE6.
> Fix it by removing the extra dce_clk_mgr_construct.
>
> Fixes: 62eab49faae7 ("drm/amd/display: hide VGH asic specific structs")
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> index 33b9d36619ff..4071851f9e86 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/clk_mgr.c
> @@ -158,7 +158,6 @@ struct clk_mgr *dc_clk_mgr_create(struct dc_context *ctx, struct pp_smu_funcs *p
>                         return NULL;
>                 }
>                 dce60_clk_mgr_construct(ctx, clk_mgr);
> -               dce_clk_mgr_construct(ctx, clk_mgr);
>                 return &clk_mgr->base;
>         }
>  #endif
> --
> 2.50.1
>

Re: [PATCH 3/3] drm/amd/display: Fix DCE 6.0 and 6.4 PLL programming.

[Alex Deucher]

Applied patches 2 and 3.  Is 1 still needed with the other patch Alex mentioned?

Thanks!

Alex

On Tue, Jul 22, 2025 at 12:23 PM Timur Kristóf <timur.kristof@gmail.com> wrote:
>
> Apparently, both DCE 6.0 and 6.4 have 3 PLLs, but PLL0 can only
> be used for DP. Make sure to initialize the correct amount of PLLs
> in DC for these DCE versions and use PLL0 only for DP.
>
> Also, on DCE 6.0 and 6.4, the PLL0 needs to be powered on at
> initialization as opposed to DCE 6.1 and 7.x which use a different
> clock source for DFS.
>
> The following functions were used as reference from the old
> radeon driver implementation of DCE 6.x:
> - radeon_atom_pick_pll
> - atombios_crtc_set_disp_eng_pll
>
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>  .../display/dc/clk_mgr/dce100/dce_clk_mgr.c   |  5 +++
>  .../dc/resource/dce60/dce60_resource.c        | 34 +++++++++++--------
>  2 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> index 26feefbb8990..f5ad0a177038 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> @@ -245,6 +245,11 @@ int dce_set_clock(
>         pxl_clk_params.target_pixel_clock_100hz = requested_clk_khz * 10;
>         pxl_clk_params.pll_id = CLOCK_SOURCE_ID_DFS;
>
> +       /* DCE 6.0, DCE 6.4: engine clock is the same as PLL0 */
> +       if (clk_mgr_base->ctx->dce_version == DCE_VERSION_6_0 ||
> +           clk_mgr_base->ctx->dce_version == DCE_VERSION_6_4)
> +               pxl_clk_params.pll_id = CLOCK_SOURCE_ID_PLL0;
> +
>         if (clk_mgr_dce->dfs_bypass_active)
>                 pxl_clk_params.flags.SET_DISPCLK_DFS_BYPASS = true;
>
> diff --git a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> index 58b59d52dc9d..53b60044653f 100644
> --- a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> @@ -373,7 +373,7 @@ static const struct resource_caps res_cap = {
>                 .num_timing_generator = 6,
>                 .num_audio = 6,
>                 .num_stream_encoder = 6,
> -               .num_pll = 2,
> +               .num_pll = 3,
>                 .num_ddc = 6,
>  };
>
> @@ -389,7 +389,7 @@ static const struct resource_caps res_cap_64 = {
>                 .num_timing_generator = 2,
>                 .num_audio = 2,
>                 .num_stream_encoder = 2,
> -               .num_pll = 2,
> +               .num_pll = 3,
>                 .num_ddc = 2,
>  };
>
> @@ -973,21 +973,24 @@ static bool dce60_construct(
>
>         if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
>
> +               /* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
>                 pool->base.clock_sources[1] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
>                 pool->base.clk_src_count = 2;
>
>         } else {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
>
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> -               pool->base.clk_src_count = 1;
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +               pool->base.clock_sources[1] =
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> +               pool->base.clk_src_count = 2;
>         }
>
>         if (pool->base.dp_clock_source == NULL) {
> @@ -1365,21 +1368,24 @@ static bool dce64_construct(
>
>         if (bp->fw_info_valid && bp->fw_info.external_clock_source_frequency_for_dp != 0) {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
>
> +               /* DCE 6.0 and 6.4: PLL0 can only be used with DP. Don't initialize it here. */
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
>                 pool->base.clock_sources[1] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
>                 pool->base.clk_src_count = 2;
>
>         } else {
>                 pool->base.dp_clock_source =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], true);
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
>
>                 pool->base.clock_sources[0] =
> -                               dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> -               pool->base.clk_src_count = 1;
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> +               pool->base.clock_sources[1] =
> +                       dce60_clock_source_create(ctx, bp, CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> +               pool->base.clk_src_count = 2;
>         }
>
>         if (pool->base.dp_clock_source == NULL) {
> --
> 2.50.1
>

Re: [PATCH 3/3] drm/amd/display: Fix DCE 6.0 and 6.4 PLL programming.

[Timur Kristóf]

Hi,

Thanks for the quick review!

No, I think patch 1 isn't going to be needed if the issue is already
being addressed by the other one.

Timur

On Tue, 2025-07-29 at 09:50 -0400, Alex Deucher wrote:
> Applied patches 2 and 3.  Is 1 still needed with the other patch Alex
> mentioned?
> 
> Thanks!
> 
> Alex
> 
> On Tue, Jul 22, 2025 at 12:23 PM Timur Kristóf
> <timur.kristof@gmail.com> wrote:
> > 
> > Apparently, both DCE 6.0 and 6.4 have 3 PLLs, but PLL0 can only
> > be used for DP. Make sure to initialize the correct amount of PLLs
> > in DC for these DCE versions and use PLL0 only for DP.
> > 
> > Also, on DCE 6.0 and 6.4, the PLL0 needs to be powered on at
> > initialization as opposed to DCE 6.1 and 7.x which use a different
> > clock source for DFS.
> > 
> > The following functions were used as reference from the old
> > radeon driver implementation of DCE 6.x:
> > - radeon_atom_pick_pll
> > - atombios_crtc_set_disp_eng_pll
> > 
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >  .../display/dc/clk_mgr/dce100/dce_clk_mgr.c   |  5 +++
> >  .../dc/resource/dce60/dce60_resource.c        | 34 +++++++++++----
> > ----
> >  2 files changed, 25 insertions(+), 14 deletions(-)
> > 
> > diff --git
> > a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> > b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> > index 26feefbb8990..f5ad0a177038 100644
> > --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> > +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c
> > @@ -245,6 +245,11 @@ int dce_set_clock(
> >         pxl_clk_params.target_pixel_clock_100hz = requested_clk_khz
> > * 10;
> >         pxl_clk_params.pll_id = CLOCK_SOURCE_ID_DFS;
> > 
> > +       /* DCE 6.0, DCE 6.4: engine clock is the same as PLL0 */
> > +       if (clk_mgr_base->ctx->dce_version == DCE_VERSION_6_0 ||
> > +           clk_mgr_base->ctx->dce_version == DCE_VERSION_6_4)
> > +               pxl_clk_params.pll_id = CLOCK_SOURCE_ID_PLL0;
> > +
> >         if (clk_mgr_dce->dfs_bypass_active)
> >                 pxl_clk_params.flags.SET_DISPCLK_DFS_BYPASS = true;
> > 
> > diff --git
> > a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> > b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> > index 58b59d52dc9d..53b60044653f 100644
> > ---
> > a/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> > +++
> > b/drivers/gpu/drm/amd/display/dc/resource/dce60/dce60_resource.c
> > @@ -373,7 +373,7 @@ static const struct resource_caps res_cap = {
> >                 .num_timing_generator = 6,
> >                 .num_audio = 6,
> >                 .num_stream_encoder = 6,
> > -               .num_pll = 2,
> > +               .num_pll = 3,
> >                 .num_ddc = 6,
> >  };
> > 
> > @@ -389,7 +389,7 @@ static const struct resource_caps res_cap_64 =
> > {
> >                 .num_timing_generator = 2,
> >                 .num_audio = 2,
> >                 .num_stream_encoder = 2,
> > -               .num_pll = 2,
> > +               .num_pll = 3,
> >                 .num_ddc = 2,
> >  };
> > 
> > @@ -973,21 +973,24 @@ static bool dce60_construct(
> > 
> >         if (bp->fw_info_valid && bp-
> > >fw_info.external_clock_source_frequency_for_dp != 0) {
> >                 pool->base.dp_clock_source =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> > 
> > +               /* DCE 6.0 and 6.4: PLL0 can only be used with DP.
> > Don't initialize it here. */
> >                 pool->base.clock_sources[0] =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], false);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> >                 pool->base.clock_sources[1] =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> >                 pool->base.clk_src_count = 2;
> > 
> >         } else {
> >                 pool->base.dp_clock_source =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
> > 
> >                 pool->base.clock_sources[0] =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> > -               pool->base.clk_src_count = 1;
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> > +               pool->base.clock_sources[1] =
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> > +               pool->base.clk_src_count = 2;
> >         }
> > 
> >         if (pool->base.dp_clock_source == NULL) {
> > @@ -1365,21 +1368,24 @@ static bool dce64_construct(
> > 
> >         if (bp->fw_info_valid && bp-
> > >fw_info.external_clock_source_frequency_for_dp != 0) {
> >                 pool->base.dp_clock_source =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_EXTERNAL, NULL, true);
> > 
> > +               /* DCE 6.0 and 6.4: PLL0 can only be used with DP.
> > Don't initialize it here. */
> >                 pool->base.clock_sources[0] =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], false);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> >                 pool->base.clock_sources[1] =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> >                 pool->base.clk_src_count = 2;
> > 
> >         } else {
> >                 pool->base.dp_clock_source =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[0], true);
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL0, &clk_src_regs[0], true);
> > 
> >                 pool->base.clock_sources[0] =
> > -                               dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL2, &clk_src_regs[1], false);
> > -               pool->base.clk_src_count = 1;
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL1, &clk_src_regs[1], false);
> > +               pool->base.clock_sources[1] =
> > +                       dce60_clock_source_create(ctx, bp,
> > CLOCK_SOURCE_ID_PLL2, &clk_src_regs[2], false);
> > +               pool->base.clk_src_count = 2;
> >         }
> > 
> >         if (pool->base.dp_clock_source == NULL) {
> > --
> > 2.50.1
> >