Re: [RFC PATCH 18/39] drm/i915/display: Determine Cx0 PLL DP mode from PLL state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 01, 2025 at 12:16:57PM +0300, Jani Nikula wrote:
> On Wed, 01 Oct 2025, Mika Kahola <mika.kahola@intel.com> wrote:
> > From: Imre Deak <imre.deak@intel.com>
> >
> > 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 <imre.deak@intel.com>
> > ---
> >  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