On Thu, 16 Oct 2025, Ville Syrjälä <[email protected]> wrote: > 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.
Okay, yuck, but acceptable for consistency in the *PLL* state. As long as we're careful to not let that leak to crtc state. BR, Jani. -- Jani Nikula, Intel
