> Subject: RE: [PATCH 1/3] drm/i915/cx0: Split PLL enabling/disabling in two > parts > > 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? >
Any inputs here @Kahola, Mika @Deak, Imre Regards, Suraj Kandpal > -- > 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 > >> >
