On Wed, Oct 01, 2025 at 12:16:57PM +0300, Jani Nikula wrote: > On Wed, 01 Oct 2025, Mika Kahola <[email protected]> wrote: > > From: Imre Deak <[email protected]> > > > > The Cx0 PLL enable programming needs to know if the PLL is in DP or HDMI > > mode. The PLL manager framework doesn't pass the CRTC state to the PLL's > > enable hook, so prepare here for the conversion to use the PLL manager > > for Cx0 PHY PLLs by determining the DP/HDMI mode from the PLL state. > > > > For C10 PHYs use the fact that the HDMI divider value in the PLL > > registers are set if and only if the PLL is in HDMI mode. > > > > For C20 PHYs use the DP mode flag programmed to the VDR SERDES register, > > which is set if and only if the PLL is in DP mode. > > > > Assert that the above PLL/VDR SERDES register values match the DP/HDMI > > mode being configured already during state computation. > > > > This also allows dropping the is_dp param from the > > __intel_cx0pll_enable() function, since it can retrieve this now from > > the PLL state. > > > > Signed-off-by: Imre Deak <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 43 ++++++++++++++++---- > > 1 file changed, 36 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > index 93e37b7ac3d9..f2fd766343d5 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > @@ -2090,6 +2090,24 @@ static void intel_c10pll_update_pll(struct > > intel_encoder *encoder, > > pll_state->c10.pll[i] = 0; > > } > > > > +static bool c10pll_state_is_dp(const struct intel_c10pll_state *pll_state) > > +{ > > + return !REG_FIELD_GET8(C10_PLL15_HDMIDIV_MASK, pll_state->pll[15]); > > +} > > + > > +static bool c20pll_state_is_dp(const struct intel_c20pll_state *pll_state) > > +{ > > + return pll_state->vdr.serdes_rate & PHY_C20_IS_DP; > > Wouldn't need this if software state was the logical state instead of > hardware state that you need to mask. It could be just > pll_state->vdr.is_dp, and no function needed.
I think for now we want the PLL states to be raw registers. That's how it all worked so far, except that snps/cx0 screwed that up by adding some non-register stuff in there. It looks to me like one of the reasons why the cx0 code is a bit of a mess is exactly because it hasn't fully committed to either representation. I think for now we should generally nuke that abstract stuff from cx0/snps and go for pure register values to keep the design consistent. In the future we might want to change things to track the PLL state in some common abstract form in high level code, and just convert to/from the register based representation inside the specific PLL implementation. For that we would need to come up with some abstract PLL state that covers all the important aspects across all the platforms, but doesn't delve into the super low level hw details because there's just far too much of that in PLLs. -- Ville Syrjälä Intel
