The .enable callback in struct intel_dpll_funcs returns void, providing no way to report PLL enable failures to callers. This leaves _intel_enable_shared_dpll() and intel_dpll_enable() unable to detect when a PLL fails to lock, causing pll->on to be set to true unconditionally and the CRTC enable sequence to proceed against a non-functional PLL.
Change the .enable callback to return int. Update all implementations to return 0 (no functional change for platforms where enable cannot fail). Thread the error through _intel_enable_shared_dpll() and intel_dpll_enable(), rolling back active_mask and power domain state on failure. Update hsw_crtc_enable() and ilk_pch_enable() to check the return value and bail out before attempting to drive a pipe with no working PLL. No functional change on any platform yet, as all .enable callbacks return 0. A subsequent patch will make the CX0 PHY PLL enable path return errors on failure. Signed-off-by: Aaron Esau <[email protected]> --- drivers/gpu/drm/i915/display/intel_display.c | 10 ++- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 87 ++++++++++++++----- drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 2 +- .../gpu/drm/i915/display/intel_pch_display.c | 7 +- 4 files changed, 80 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 0f82bf771..74bfeed31 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1645,8 +1645,14 @@ static void hsw_crtc_enable(struct intel_atomic_state *state, intel_encoders_pre_pll_enable(state, crtc); - if (new_crtc_state->intel_dpll) - intel_dpll_enable(new_crtc_state); + if (new_crtc_state->intel_dpll) { + if (intel_dpll_enable(new_crtc_state)) { + drm_err(display->drm, + "[CRTC:%d:%s] PLL enable failed, aborting crtc enable\n", + crtc->base.base.id, crtc->base.name); + return; + } + } intel_encoders_pre_enable(state, crtc); diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index 9aa84a430..78fd2e5f9 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -69,9 +69,9 @@ struct intel_dpll_funcs { * Hook for enabling the pll, called from intel_enable_dpll() if * the pll is not already enabled. */ - void (*enable)(struct intel_display *display, - struct intel_dpll *pll, - const struct intel_dpll_hw_state *dpll_hw_state); + int (*enable)(struct intel_display *display, + struct intel_dpll *pll, + const struct intel_dpll_hw_state *dpll_hw_state); /* * Hook for disabling the pll, called from intel_disable_dpll() @@ -245,14 +245,28 @@ intel_tc_pll_enable_reg(struct intel_display *display, return MG_PLL_ENABLE(tc_port); } -static void _intel_enable_shared_dpll(struct intel_display *display, - struct intel_dpll *pll) +static int _intel_enable_shared_dpll(struct intel_display *display, + struct intel_dpll *pll) { + int ret; + if (pll->info->power_domain) pll->wakeref = intel_display_power_get(display, pll->info->power_domain); - pll->info->funcs->enable(display, pll, &pll->state.hw_state); + ret = pll->info->funcs->enable(display, pll, &pll->state.hw_state); + if (ret) { + drm_err(display->drm, "%s: PLL enable failed (err %d)\n", + pll->info->name, ret); + pll->on = false; + if (pll->info->power_domain) + intel_display_power_put(display, pll->info->power_domain, + pll->wakeref); + return ret; + } + pll->on = true; + + return 0; } static void _intel_disable_shared_dpll(struct intel_display *display, @@ -270,17 +284,20 @@ static void _intel_disable_shared_dpll(struct intel_display *display, * @crtc_state: CRTC, and its state, which has a DPLL * * Enable DPLL used by @crtc. + * + * Returns: 0 on success, negative error code on failure. */ -void intel_dpll_enable(const struct intel_crtc_state *crtc_state) +int intel_dpll_enable(const struct intel_crtc_state *crtc_state) { struct intel_display *display = to_intel_display(crtc_state); struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct intel_dpll *pll = crtc_state->intel_dpll; unsigned int pipe_mask = intel_crtc_joined_pipe_mask(crtc_state); unsigned int old_mask; + int ret = 0; if (drm_WARN_ON(display->drm, !pll)) - return; + return -EINVAL; mutex_lock(&display->dpll.lock); old_mask = pll->active_mask; @@ -305,10 +322,14 @@ void intel_dpll_enable(const struct intel_crtc_state *crtc_state) drm_dbg_kms(display->drm, "enabling %s\n", pll->info->name); - _intel_enable_shared_dpll(display, pll); + ret = _intel_enable_shared_dpll(display, pll); + if (ret) + pll->active_mask &= ~pipe_mask; out: mutex_unlock(&display->dpll.lock); + + return ret; } /** @@ -577,7 +598,7 @@ static void ibx_assert_pch_refclk_enabled(struct intel_display *display) "PCH refclk assertion failure, should be active but is disabled\n"); } -static void ibx_pch_dpll_enable(struct intel_display *display, +static int ibx_pch_dpll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -604,6 +625,8 @@ static void ibx_pch_dpll_enable(struct intel_display *display, intel_de_write(display, PCH_DPLL(id), hw_state->dpll); intel_de_posting_read(display, PCH_DPLL(id)); udelay(200); + + return 0; } static void ibx_pch_dpll_disable(struct intel_display *display, @@ -707,7 +730,7 @@ static const struct intel_dpll_mgr pch_pll_mgr = { .compare_hw_state = ibx_compare_hw_state, }; -static void hsw_ddi_wrpll_enable(struct intel_display *display, +static int hsw_ddi_wrpll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -717,9 +740,11 @@ static void hsw_ddi_wrpll_enable(struct intel_display *display, intel_de_write(display, WRPLL_CTL(id), hw_state->wrpll); intel_de_posting_read(display, WRPLL_CTL(id)); udelay(20); + + return 0; } -static void hsw_ddi_spll_enable(struct intel_display *display, +static int hsw_ddi_spll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -728,6 +753,8 @@ static void hsw_ddi_spll_enable(struct intel_display *display, intel_de_write(display, SPLL_CTL, hw_state->spll); intel_de_posting_read(display, SPLL_CTL); udelay(20); + + return 0; } static void hsw_ddi_wrpll_disable(struct intel_display *display, @@ -1300,10 +1327,11 @@ static const struct intel_dpll_funcs hsw_ddi_spll_funcs = { .get_freq = hsw_ddi_spll_get_freq, }; -static void hsw_ddi_lcpll_enable(struct intel_display *display, +static int hsw_ddi_lcpll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *hw_state) { + return 0; } static void hsw_ddi_lcpll_disable(struct intel_display *display, @@ -1393,7 +1421,7 @@ static void skl_ddi_pll_write_ctrl1(struct intel_display *display, intel_de_posting_read(display, DPLL_CTRL1); } -static void skl_ddi_pll_enable(struct intel_display *display, +static int skl_ddi_pll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -1413,15 +1441,19 @@ static void skl_ddi_pll_enable(struct intel_display *display, if (intel_de_wait_for_set_ms(display, DPLL_STATUS, DPLL_LOCK(id), 5)) drm_err(display->drm, "DPLL %d not locked\n", id); + + return 0; } -static void skl_ddi_dpll0_enable(struct intel_display *display, +static int skl_ddi_dpll0_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { const struct skl_dpll_hw_state *hw_state = &dpll_hw_state->skl; skl_ddi_pll_write_ctrl1(display, pll, hw_state); + + return 0; } static void skl_ddi_pll_disable(struct intel_display *display, @@ -2053,7 +2085,7 @@ static const struct intel_dpll_mgr skl_pll_mgr = { .compare_hw_state = skl_compare_hw_state, }; -static void bxt_ddi_pll_enable(struct intel_display *display, +static int bxt_ddi_pll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -2158,6 +2190,8 @@ static void bxt_ddi_pll_enable(struct intel_display *display, temp &= ~LANESTAGGER_STRAP_OVRD; temp |= hw_state->pcsdw12; intel_de_write(display, BXT_PORT_PCS_DW12_GRP(phy, ch), temp); + + return 0; } static void bxt_ddi_pll_disable(struct intel_display *display, @@ -4007,7 +4041,7 @@ static void adlp_cmtg_clock_gating_wa(struct intel_display *display, struct inte drm_dbg_kms(display->drm, "Unexpected flags in TRANS_CMTG_CHICKEN: %08x\n", val); } -static void combo_pll_enable(struct intel_display *display, +static int combo_pll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -4029,9 +4063,11 @@ static void combo_pll_enable(struct intel_display *display, adlp_cmtg_clock_gating_wa(display, pll); /* DVFS post sequence would be here. See the comment above. */ + + return 0; } -static void icl_tbt_pll_enable(struct intel_display *display, +static int icl_tbt_pll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -4050,9 +4086,11 @@ static void icl_tbt_pll_enable(struct intel_display *display, icl_pll_enable(display, pll, TBT_PLL_ENABLE); /* DVFS post sequence would be here. See the comment above. */ + + return 0; } -static void mg_pll_enable(struct intel_display *display, +static int mg_pll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { @@ -4075,6 +4113,8 @@ static void mg_pll_enable(struct intel_display *display, icl_pll_enable(display, pll, enable_reg); /* DVFS post sequence would be here. See the comment above. */ + + return 0; } static void icl_pll_disable(struct intel_display *display, @@ -4392,16 +4432,18 @@ static int mtl_pll_get_freq(struct intel_display *display, return intel_cx0pll_calc_port_clock(encoder, &dpll_hw_state->cx0pll); } -static void mtl_pll_enable(struct intel_display *display, +static int mtl_pll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *dpll_hw_state) { struct intel_encoder *encoder = get_intel_encoder(display, pll); if (drm_WARN_ON(display->drm, !encoder)) - return; + return -ENODEV; intel_mtl_pll_enable(encoder, pll, dpll_hw_state); + + return 0; } static void mtl_pll_disable(struct intel_display *display, @@ -4422,10 +4464,11 @@ static const struct intel_dpll_funcs mtl_pll_funcs = { .get_freq = mtl_pll_get_freq, }; -static void mtl_tbt_pll_enable(struct intel_display *display, +static int mtl_tbt_pll_enable(struct intel_display *display, struct intel_dpll *pll, const struct intel_dpll_hw_state *hw_state) { + return 0; } static void mtl_tbt_pll_disable(struct intel_display *display, diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h index 5b71c8605..21fae6fd0 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h @@ -435,7 +435,7 @@ int intel_dpll_get_freq(struct intel_display *display, bool intel_dpll_get_hw_state(struct intel_display *display, struct intel_dpll *pll, struct intel_dpll_hw_state *dpll_hw_state); -void intel_dpll_enable(const struct intel_crtc_state *crtc_state); +int intel_dpll_enable(const struct intel_crtc_state *crtc_state); void intel_dpll_disable(const struct intel_crtc_state *crtc_state); void intel_dpll_swap_state(struct intel_atomic_state *state); void intel_dpll_init(struct intel_display *display); diff --git a/drivers/gpu/drm/i915/display/intel_pch_display.c b/drivers/gpu/drm/i915/display/intel_pch_display.c index 16619f7be..cb979a946 100644 --- a/drivers/gpu/drm/i915/display/intel_pch_display.c +++ b/drivers/gpu/drm/i915/display/intel_pch_display.c @@ -399,7 +399,12 @@ void ilk_pch_enable(struct intel_atomic_state *state, * get_dpll unconditionally resets the pll - we need that * to have the right LVDS enable sequence. */ - intel_dpll_enable(crtc_state); + if (intel_dpll_enable(crtc_state)) { + drm_err(display->drm, + "[CRTC:%d:%s] PCH PLL enable failed, aborting PCH enable\n", + crtc->base.base.id, crtc->base.name); + return; + } /* set transcoder timing, panel must allow it */ assert_pps_unlocked(display, pipe); -- 2.54.0
