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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 16 Oct 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

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