Quoting Kandpal, Suraj (2025-12-31 02:07:35-03:00) >> Subject: Re: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two >> parts >> >> Quoting Suraj Kandpal (2025-12-30 05:31:40-03:00) >> >From: Mika Kahola <[email protected]> >> > >> >Split PLL enabling/disabling in two parts - one for pll setting pll >> >dividers and second one to enable/disable pll clock. PLL clock >> >enabling/disbling happens via encoder->enable_clock/disable_clock >> >function hook. The reason for doing this is that we need to make sure >> >the clock enablement happens after PPS ON step to be inline with the >> >sequences which we end up violating otherwise. As a result of this >> >violation we end up in a hanged state if machine stays idle for more >> >that 15 mins. >> >> So, it appears this started happening when we Cx0 code was integrated into >> the DPLL framework and then the driver started enabling the PHY PLL/clock >> too early, right? >> >> I am lacking some context/background here due to my unfamiliarity with pre- >> MTL platforms, but why I exactly do we program the PLLs before the modeset >> sequence? Is it related to the shared nature of PLLs for platforms pre- >> C10/pre-C20? If so, do we really need to do the same for >> C10/C20 PHYs, since we have dedicated PLLs for them? >> >> (Sorry for asking here and a bit too late. Probably the better place to ask >> this >> was in series that integrated Cx0 into the DPLL framework.) > > >Right it used to be actually because of the shared nature of PLL's. With c10 >c20 we moved >to a different framework where we called the the sequence together using hooks >like enable_clock >and disable_clock since there was not a lot of time of time to refactor the >dpll_shared_framework to >a framework with supported individual ones. >Now that we had time we shifted cx0 back to the previous framework but missed >defer the clock enablement
Then, if we move forward with this, perhaps this patch deserves a "Fixes:" trailer. >To later during enable clock time so that we honor the sequence, why we had to >do this is even though its not shared PLL anymore is >To make sure this framework is backward compatible too. I see. If the requirement for programming PLL parameters early was only because of shared PLLs and we do not have that same requirements for C10/C20, I would argue that doing the whole programming at once and only during the "enable clock" phase of the encoder would make the driver more compliant with the Bspec. I also noticed that, for the older displays, the "enable clock" thing is the part that selects the PLL (which is already enabled) as the port's "clock source". With C10/C20 we are actually deferring the PLL enabling to the "enable clock" phase of the port while, I believe, the expectation of intel_dpll_funcs::enable() is that the PLL would be enabled when the function returned, which would not be exactly true for C10/C20 after this patch. What if, in intel_dpll_mgr, we made the distinction between enabling the PLL early and enabling it at "intel_ddi_enable_clock()" time? -- Gustavo Sousa >Also we had to move cx0 pll framework back to dpll framework because the >previous can work well as long as the ports are static hence aren’t >As future proof , we plan to move LT PHY back here too once this ages well. > >> >> > >> >PLL state verification happens now earlier than the clock is enabled >> >which causes a drm warn to be thrown. Silence this warning by allowing >> >this check for only earlier platforms than MeteorLake. >> > >> >Bspec: 49190 >> >> This Bspec page is not invalid for platforms using C10/C20 PHYs. >> >> We probably want to use these instead: >> >> Bspec: 65448, 68849 >> > >Sure will replace them. > >Regards, >Suraj Kandpal > >> -- >> Gustavo Sousa >> >> >Signed-off-by: Mika Kahola <[email protected]> >> >Signed-off-by: Suraj Kandpal <[email protected]> >> >--- >> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 87 ++++++++++++------- >> >drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 12 +-- >> > 2 files changed, 64 insertions(+), 35 deletions(-) >> > >> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c >> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c >> >index 7288065d2461..f3baba264e88 100644 >> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c >> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c >> >@@ -3225,11 +3225,8 @@ static void intel_cx0pll_enable(struct >> >intel_encoder *encoder, { >> > int port_clock = pll_state->use_c10 ? pll_state->c10.clock : >> > pll_state- >> >c20.clock; >> > struct intel_display *display = to_intel_display(encoder); >> >- enum phy phy = intel_encoder_to_phy(encoder); >> > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); >> > bool lane_reversal = dig_port->lane_reversal; >> >- u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 : >> >- INTEL_CX0_LANE0; >> > struct ref_tracker *wakeref = >> >intel_cx0_phy_transaction_begin(encoder); >> > >> > /* >> >@@ -3284,27 +3281,6 @@ static void intel_cx0pll_enable(struct >> intel_encoder *encoder, >> > */ >> > intel_de_write(display, DDI_CLK_VALFREQ(encoder->port), >> >port_clock); >> > >> >- /* >> >- * 9. Set PORT_CLOCK_CTL register PCLK PLL Request >> >- * LN<Lane for maxPCLK> to "1" to enable PLL. >> >- */ >> >- intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder- >> >port), >> >- intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES), >> >- intel_cx0_get_pclk_pll_request(maxpclk_lane)); >> >- >> >- /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> >> == "1". */ >> >- if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, >> encoder->port), >> >- >> >intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES), >> >- intel_cx0_get_pclk_pll_ack(maxpclk_lane), >> >- XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL)) >> >- drm_warn(display->drm, "Port %c PLL not locked\n", >> >- phy_name(phy)); >> >- >> >- /* >> >- * 11. Follow the Display Voltage Frequency Switching Sequence >> >After >> >- * Frequency Change. We handle this step in bxt_set_cdclk(). >> >- */ >> >- >> > /* >> > * 12. Toggle powerdown if HDMI is enabled on C10 PHY. >> > * >> >@@ -3403,6 +3379,42 @@ static int intel_mtl_tbt_clock_select(struct >> intel_display *display, >> > } >> > } >> > >> >+static void intel_cx0pll_enable_clock(struct intel_encoder *encoder) { >> >+ struct intel_display *display = to_intel_display(encoder); >> >+ enum phy phy = intel_encoder_to_phy(encoder); >> >+ struct intel_digital_port *dig_port = enc_to_dig_port(encoder); >> >+ bool lane_reversal = dig_port->lane_reversal; >> >+ INTEL_CX0_LANE0; >> >+ u8 maxpclk_lane = lane_reversal ? INTEL_CX0_LANE1 : >> >+ INTEL_CX0_LANE0; >> >+ >> >+ struct ref_tracker *wakeref = >> >+ intel_cx0_phy_transaction_begin(encoder); >> >+ >> >+ /* >> >+ * 9. Set PORT_CLOCK_CTL register PCLK PLL Request >> >+ * LN<Lane for maxPCLK> to "1" to enable PLL. >> >+ */ >> >+ intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder- >> >port), >> >+ intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES), >> >+ intel_cx0_get_pclk_pll_request(maxpclk_lane)); >> >+ >> >+ /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> >> == "1". */ >> >+ if (intel_de_wait_us(display, XELPDP_PORT_CLOCK_CTL(display, >> encoder->port), >> >+ >> >intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES), >> >+ intel_cx0_get_pclk_pll_ack(maxpclk_lane), >> >+ XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US, NULL)) >> >+ drm_warn(display->drm, "Port %c PLL not locked\n", >> >+ phy_name(phy)); >> >+ >> >+ /* >> >+ * 11. Follow the Display Voltage Frequency Switching Sequence >> >After >> >+ * Frequency Change. We handle this step in bxt_set_cdclk(). >> >+ */ >> >+ >> >+ intel_cx0_phy_transaction_end(encoder, wakeref); } >> >+ >> > void intel_mtl_tbt_pll_enable_clock(struct intel_encoder *encoder, int >> >port_clock) { >> > struct intel_display *display = to_intel_display(encoder); @@ >> >-3472,6 +3484,8 @@ void intel_mtl_pll_enable_clock(struct intel_encoder >> >*encoder, >> > >> > if (intel_tc_port_in_tbt_alt_mode(dig_port)) >> > intel_mtl_tbt_pll_enable_clock(encoder, >> > crtc_state->port_clock); >> >+ else >> >+ intel_cx0pll_enable_clock(encoder); >> > } >> > >> > /* >> >@@ -3567,12 +3581,6 @@ static void intel_cx0pll_disable(struct >> intel_encoder *encoder) >> > * Frequency Change. We handle this step in bxt_set_cdclk(). >> > */ >> > >> >- /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. >> >*/ >> >- intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder- >> >port), >> >- XELPDP_DDI_CLOCK_SELECT_MASK(display), 0); >> >- intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder- >> >port), >> >- XELPDP_FORWARD_CLOCK_UNGATE, 0); >> >- >> > intel_cx0_phy_transaction_end(encoder, wakeref); } >> > >> >@@ -3586,6 +3594,20 @@ static bool intel_cx0_pll_is_enabled(struct >> intel_encoder *encoder) >> > intel_cx0_get_pclk_pll_request(lane); >> > } >> > >> >+static void intel_cx0pll_disable_clock(struct intel_encoder *encoder) >> >+{ >> >+ struct intel_display *display = to_intel_display(encoder); >> >+ struct ref_tracker *wakeref = >> >+intel_cx0_phy_transaction_begin(encoder); >> >+ >> >+ /* 7. Program PORT_CLOCK_CTL register to disable and gate clocks. >> >*/ >> >+ intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder- >> >port), >> >+ XELPDP_DDI_CLOCK_SELECT_MASK(display), 0); >> >+ intel_de_rmw(display, XELPDP_PORT_CLOCK_CTL(display, encoder- >> >port), >> >+ XELPDP_FORWARD_CLOCK_UNGATE, 0); >> >+ >> >+ intel_cx0_phy_transaction_end(encoder, wakeref); } >> >+ >> > void intel_mtl_tbt_pll_disable_clock(struct intel_encoder *encoder) { >> > struct intel_display *display = to_intel_display(encoder); @@ >> >-3635,6 +3657,9 @@ void intel_mtl_pll_disable_clock(struct >> >intel_encoder *encoder) >> > >> > if (intel_tc_port_in_tbt_alt_mode(dig_port)) >> > intel_mtl_tbt_pll_disable_clock(encoder); >> >+ else >> >+ intel_cx0pll_disable_clock(encoder); >> >+ >> > } >> > >> > enum icl_port_dpll_id >> >@@ -3783,6 +3808,8 @@ void intel_cx0_pll_power_save_wa(struct >> intel_display *display) >> > encoder->base.base.id, >> >encoder->base.name); >> > >> > intel_cx0pll_enable(encoder, &pll_state); >> >+ intel_cx0pll_enable_clock(encoder); >> > intel_cx0pll_disable(encoder); >> >+ intel_cx0pll_disable_clock(encoder); >> > } >> > } >> >diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> >b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> >index 9aa84a430f09..59395076103c 100644 >> >--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> >+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> >@@ -186,11 +186,13 @@ void assert_dpll(struct intel_display *display, >> > "asserting DPLL %s with no DPLL\n", >> > str_on_off(state))) >> > return; >> > >> >- cur_state = intel_dpll_get_hw_state(display, pll, &hw_state); >> >- INTEL_DISPLAY_STATE_WARN(display, cur_state != state, >> >- "%s assertion failure (expected %s, >> >current %s)\n", >> >- pll->info->name, str_on_off(state), >> >- str_on_off(cur_state)); >> >+ if (DISPLAY_VER(display) < 14) { >> >+ cur_state = intel_dpll_get_hw_state(display, pll, >> >&hw_state); >> >+ INTEL_DISPLAY_STATE_WARN(display, cur_state != state, >> >+ "%s assertion failure (expected >> >%s, current %s)\n", >> >+ pll->info->name, >> >str_on_off(state), >> >+ str_on_off(cur_state)); >> >+ } >> > } >> > >> > static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id) >> >-- >> >2.34.1 >> >
