From: Charlene Liu Charlene.Liu@amd.com
[ Upstream commit 1bcd679209420305a86833bc357d50021909edaf ]
[why] DPP CLK enable needs to disable DPPCLK RCG first. The DPPCLK_en in dccg should always be enabled when the corresponding pipe is enabled.
Reviewed-by: Hansen Dsouza hansen.dsouza@amd.com Signed-off-by: Charlene Liu Charlene.Liu@amd.com Signed-off-by: Ray Wu ray.wu@amd.com Tested-by: Daniel Wheeler daniel.wheeler@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com Signed-off-by: Sasha Levin sashal@kernel.org ---
Based on my analysis of both the commit message and code changes, my answer is: **YES** This commit should be backported to stable kernel trees for the following reasons: ## Critical Hardware Sequencing Fix The commit addresses a fundamental hardware sequencing requirement where DPP (Display Pipes and Planes) Root Clock Gating (RCG) must be disabled before enabling the DPP clock. This is a critical timing constraint that, if violated, can cause severe system issues. ## Specific Code Evidence 1. **In `dccg35_update_dpp_dto()`**, the fix adds a crucial sequencing step: ```c + dccg35_set_dppclk_rcg(dccg, dpp_inst, false); REG_SET_2(DPPCLK_DTO_PARAM[dpp_inst], 0, DPPCLK0_DTO_PHASE, phase, DPPCLK0_DTO_MODULO, modulo); dcn35_set_dppclk_enable(dccg, dpp_inst, true); ``` This ensures RCG is disabled (false) before enabling the DPP clock. 2. **In `dcn35_enable_plane()`**, explicit clock control is added: ```c + /*make sure DPPCLK is on*/ + dccg->funcs->dccg_root_gate_disable_control(dccg, dpp->inst, true); + dpp->funcs->dpp_dppclk_control(dpp, false, true); ``` 3. **Removal of problematic init code** in `dcn35_init_hw()`: ```c - if (res_pool->dccg->funcs->dccg_root_gate_disable_control) { - for (i = 0; i < res_pool->pipe_count; i++) - res_pool->dccg->funcs->dccg_root_gate_disable_control(res_pool->dccg, i, 0); - } ``` ## Similarity to Previous Critical Fixes This commit follows the pattern of Similar Commit #1, which also addressed clock gating sequencing issues that could cause system hangs. The commit message explicitly states this is needed because "DPP CLK enable needs to disable DPPCLK RCG first." ## Impact and Risk Assessment - **Bug Fixed**: Incorrect clock sequencing that can cause system instability - **Scope**: Limited to DCN35 display hardware (recent AMD GPUs) - **Complexity**: Small, focused changes with clear purpose - **Risk**: Low risk of regression - adds proper sequencing without major architectural changes This fix meets all criteria for stable backporting: - Fixes a real bug affecting users (system stability) - Small and contained changes - No new features or architectural changes - Addresses hardware constraints that cannot be worked around The commit should be backported to all stable kernels that include DCN35 display support to ensure proper display functionality and system stability for users with affected AMD GPUs.
.../amd/display/dc/dccg/dcn35/dcn35_dccg.c | 38 ++++++++++++------- .../amd/display/dc/hwss/dcn35/dcn35_hwseq.c | 21 ++++++---- 2 files changed, 38 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dccg/dcn35/dcn35_dccg.c b/drivers/gpu/drm/amd/display/dc/dccg/dcn35/dcn35_dccg.c index b363f5360818d..ad910065f463f 100644 --- a/drivers/gpu/drm/amd/display/dc/dccg/dcn35/dcn35_dccg.c +++ b/drivers/gpu/drm/amd/display/dc/dccg/dcn35/dcn35_dccg.c @@ -391,6 +391,7 @@ static void dccg35_set_dppclk_rcg(struct dccg *dccg,
struct dcn_dccg *dccg_dcn = TO_DCN_DCCG(dccg);
+ if (!dccg->ctx->dc->debug.root_clock_optimization.bits.dpp && enable) return;
@@ -411,6 +412,8 @@ static void dccg35_set_dppclk_rcg(struct dccg *dccg, BREAK_TO_DEBUGGER(); break; } + //DC_LOG_DEBUG("%s: inst(%d) DPPCLK rcg_disable: %d\n", __func__, inst, enable ? 0 : 1); + }
static void dccg35_set_dpstreamclk_rcg( @@ -1112,30 +1115,24 @@ static void dcn35_set_dppclk_enable(struct dccg *dccg, { struct dcn_dccg *dccg_dcn = TO_DCN_DCCG(dccg);
+ switch (dpp_inst) { case 0: REG_UPDATE(DPPCLK_CTRL, DPPCLK0_EN, enable); - if (dccg->ctx->dc->debug.root_clock_optimization.bits.dpp) - REG_UPDATE(DCCG_GATE_DISABLE_CNTL6, DPPCLK0_ROOT_GATE_DISABLE, enable); break; case 1: REG_UPDATE(DPPCLK_CTRL, DPPCLK1_EN, enable); - if (dccg->ctx->dc->debug.root_clock_optimization.bits.dpp) - REG_UPDATE(DCCG_GATE_DISABLE_CNTL6, DPPCLK1_ROOT_GATE_DISABLE, enable); break; case 2: REG_UPDATE(DPPCLK_CTRL, DPPCLK2_EN, enable); - if (dccg->ctx->dc->debug.root_clock_optimization.bits.dpp) - REG_UPDATE(DCCG_GATE_DISABLE_CNTL6, DPPCLK2_ROOT_GATE_DISABLE, enable); break; case 3: REG_UPDATE(DPPCLK_CTRL, DPPCLK3_EN, enable); - if (dccg->ctx->dc->debug.root_clock_optimization.bits.dpp) - REG_UPDATE(DCCG_GATE_DISABLE_CNTL6, DPPCLK3_ROOT_GATE_DISABLE, enable); break; default: break; } + //DC_LOG_DEBUG("%s: dpp_inst(%d) DPPCLK_EN = %d\n", __func__, dpp_inst, enable);
}
@@ -1163,14 +1160,18 @@ static void dccg35_update_dpp_dto(struct dccg *dccg, int dpp_inst, ASSERT(false); phase = 0xff; } + dccg35_set_dppclk_rcg(dccg, dpp_inst, false);
REG_SET_2(DPPCLK_DTO_PARAM[dpp_inst], 0, DPPCLK0_DTO_PHASE, phase, DPPCLK0_DTO_MODULO, modulo);
dcn35_set_dppclk_enable(dccg, dpp_inst, true); - } else + } else { dcn35_set_dppclk_enable(dccg, dpp_inst, false); + /*we have this in hwss: disable_plane*/ + //dccg35_set_dppclk_rcg(dccg, dpp_inst, true); + } dccg->pipe_dppclk_khz[dpp_inst] = req_dppclk; }
@@ -1182,6 +1183,7 @@ static void dccg35_set_dppclk_root_clock_gating(struct dccg *dccg, if (!dccg->ctx->dc->debug.root_clock_optimization.bits.dpp) return;
+ switch (dpp_inst) { case 0: REG_UPDATE(DCCG_GATE_DISABLE_CNTL6, DPPCLK0_ROOT_GATE_DISABLE, enable); @@ -1198,6 +1200,8 @@ static void dccg35_set_dppclk_root_clock_gating(struct dccg *dccg, default: break; } + //DC_LOG_DEBUG("%s: dpp_inst(%d) rcg: %d\n", __func__, dpp_inst, enable); + }
static void dccg35_get_pixel_rate_div( @@ -1521,28 +1525,30 @@ static void dccg35_set_physymclk_root_clock_gating( switch (phy_inst) { case 0: REG_UPDATE(DCCG_GATE_DISABLE_CNTL2, - PHYASYMCLK_ROOT_GATE_DISABLE, enable ? 1 : 0); + PHYASYMCLK_ROOT_GATE_DISABLE, enable ? 0 : 1); break; case 1: REG_UPDATE(DCCG_GATE_DISABLE_CNTL2, - PHYBSYMCLK_ROOT_GATE_DISABLE, enable ? 1 : 0); + PHYBSYMCLK_ROOT_GATE_DISABLE, enable ? 0 : 1); break; case 2: REG_UPDATE(DCCG_GATE_DISABLE_CNTL2, - PHYCSYMCLK_ROOT_GATE_DISABLE, enable ? 1 : 0); + PHYCSYMCLK_ROOT_GATE_DISABLE, enable ? 0 : 1); break; case 3: REG_UPDATE(DCCG_GATE_DISABLE_CNTL2, - PHYDSYMCLK_ROOT_GATE_DISABLE, enable ? 1 : 0); + PHYDSYMCLK_ROOT_GATE_DISABLE, enable ? 0 : 1); break; case 4: REG_UPDATE(DCCG_GATE_DISABLE_CNTL2, - PHYESYMCLK_ROOT_GATE_DISABLE, enable ? 1 : 0); + PHYESYMCLK_ROOT_GATE_DISABLE, enable ? 0 : 1); break; default: BREAK_TO_DEBUGGER(); return; } + //DC_LOG_DEBUG("%s: dpp_inst(%d) PHYESYMCLK_ROOT_GATE_DISABLE:\n", __func__, phy_inst, enable ? 0 : 1); + }
static void dccg35_set_physymclk( @@ -1643,6 +1649,8 @@ static void dccg35_dpp_root_clock_control( return;
if (clock_on) { + dccg35_set_dppclk_rcg(dccg, dpp_inst, false); + /* turn off the DTO and leave phase/modulo at max */ dcn35_set_dppclk_enable(dccg, dpp_inst, 1); REG_SET_2(DPPCLK_DTO_PARAM[dpp_inst], 0, @@ -1654,6 +1662,8 @@ static void dccg35_dpp_root_clock_control( REG_SET_2(DPPCLK_DTO_PARAM[dpp_inst], 0, DPPCLK0_DTO_PHASE, 0, DPPCLK0_DTO_MODULO, 1); + /*we have this in hwss: disable_plane*/ + //dccg35_set_dppclk_rcg(dccg, dpp_inst, true); }
dccg->dpp_clock_gated[dpp_inst] = !clock_on; diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c b/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c index 922b8d71cf1aa..63077c1fad859 100644 --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c @@ -241,11 +241,6 @@ void dcn35_init_hw(struct dc *dc) dc->res_pool->hubbub->funcs->allow_self_refresh_control(dc->res_pool->hubbub, !dc->res_pool->hubbub->ctx->dc->debug.disable_stutter); } - if (res_pool->dccg->funcs->dccg_root_gate_disable_control) { - for (i = 0; i < res_pool->pipe_count; i++) - res_pool->dccg->funcs->dccg_root_gate_disable_control(res_pool->dccg, i, 0); - } - for (i = 0; i < res_pool->audio_count; i++) { struct audio *audio = res_pool->audios[i];
@@ -901,12 +896,18 @@ void dcn35_init_pipes(struct dc *dc, struct dc_state *context) void dcn35_enable_plane(struct dc *dc, struct pipe_ctx *pipe_ctx, struct dc_state *context) { + struct dpp *dpp = pipe_ctx->plane_res.dpp; + struct dccg *dccg = dc->res_pool->dccg; + + /* enable DCFCLK current DCHUB */ pipe_ctx->plane_res.hubp->funcs->hubp_clk_cntl(pipe_ctx->plane_res.hubp, true);
/* initialize HUBP on power up */ pipe_ctx->plane_res.hubp->funcs->hubp_init(pipe_ctx->plane_res.hubp); - + /*make sure DPPCLK is on*/ + dccg->funcs->dccg_root_gate_disable_control(dccg, dpp->inst, true); + dpp->funcs->dpp_dppclk_control(dpp, false, true); /* make sure OPP_PIPE_CLOCK_EN = 1 */ pipe_ctx->stream_res.opp->funcs->opp_pipe_clock_control( pipe_ctx->stream_res.opp, @@ -923,6 +924,7 @@ void dcn35_enable_plane(struct dc *dc, struct pipe_ctx *pipe_ctx, // Program system aperture settings pipe_ctx->plane_res.hubp->funcs->hubp_set_vm_system_aperture_settings(pipe_ctx->plane_res.hubp, &apt); } + //DC_LOG_DEBUG("%s: dpp_inst(%d) =\n", __func__, dpp->inst);
if (!pipe_ctx->top_pipe && pipe_ctx->plane_state @@ -938,6 +940,8 @@ void dcn35_plane_atomic_disable(struct dc *dc, struct pipe_ctx *pipe_ctx) { struct hubp *hubp = pipe_ctx->plane_res.hubp; struct dpp *dpp = pipe_ctx->plane_res.dpp; + struct dccg *dccg = dc->res_pool->dccg; +
dc->hwss.wait_for_mpcc_disconnect(dc, dc->res_pool, pipe_ctx);
@@ -955,7 +959,8 @@ void dcn35_plane_atomic_disable(struct dc *dc, struct pipe_ctx *pipe_ctx) hubp->funcs->hubp_clk_cntl(hubp, false);
dpp->funcs->dpp_dppclk_control(dpp, false, false); -/*to do, need to support both case*/ + dccg->funcs->dccg_root_gate_disable_control(dccg, dpp->inst, false); + hubp->power_gated = true;
hubp->funcs->hubp_reset(hubp); @@ -967,6 +972,8 @@ void dcn35_plane_atomic_disable(struct dc *dc, struct pipe_ctx *pipe_ctx) pipe_ctx->top_pipe = NULL; pipe_ctx->bottom_pipe = NULL; pipe_ctx->plane_state = NULL; + //DC_LOG_DEBUG("%s: dpp_inst(%d)=\n", __func__, dpp->inst); + }
void dcn35_disable_plane(struct dc *dc, struct dc_state *state, struct pipe_ctx *pipe_ctx)