| Message ID | 20250106040821.251114-2-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | SSC enablement in port clock programming | expand |
On Mon, Jan 06, 2025 at 09:38:20AM +0530, Suraj Kandpal wrote: > SSC for PLL_A is enabled for UHBR10 or UHBR20 regardless of the > need for SSC. This means the ssc_enabled variable had no say > to determine enablement of SSC on PLL A. I don't see the above in the spec. It suggests that SSC should be enabled on PLL A for MFD, but in any case SSC can only be enabled if the sink supports SSC, as indicated by dpll_hw_state.cx0pll.ssc_enabled. > Bspec: 64568, 74165, 74489, 74491 > Fixes: 237e7be0bf57 ("drm/i915/mtl: For DP2.0 10G and 20G rates use MPLLA") > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > index e768dc6a15b3..3fd959a2773c 100644 > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > @@ -2747,7 +2747,7 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder, > /* TODO: HDMI FRL */ > /* DP2.0 10G and 20G rates enable MPLLA*/ > if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == 2000000) > - val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0; > + val |= XELPDP_SSC_ENABLE_PLLA; > else > val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0; > > -- > 2.34.1 >
> -----Original Message----- > From: Deak, Imre <imre.deak@intel.com> > Sent: Tuesday, January 7, 2025 8:33 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Nautiyal, > Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma > <uma.shankar@intel.com> > Subject: Re: [PATCH 1/2] drm/i915/cx0: Fix SSC enablement in > PORT_CLOCK_CTL > > On Mon, Jan 06, 2025 at 09:38:20AM +0530, Suraj Kandpal wrote: > > SSC for PLL_A is enabled for UHBR10 or UHBR20 regardless of the need > > for SSC. This means the ssc_enabled variable had no say to determine > > enablement of SSC on PLL A. > > I don't see the above in the spec. It suggests that SSC should be enabled on > PLL A for MFD, but in any case SSC can only be enabled if the sink supports > SSC, as indicated by dpll_hw_state.cx0pll.ssc_enabled. Hi Imre, You are right In Bspec 74489 under Non-thunderbolt PLL Enable Sequence It says SSC enable to be done on PLLA for MFD when on UHBR10 or UHBR20 (PLLA is only used for C20 PHY UHBR10 and 20.) and check for ssc_enabled for Native mode to enable SSC but now the issue is that we aren't checking for MFD mode any particular reason for this ? and how would we go about checking if we are in MFD mode or not ? Also the ssc_enabled bool variable never actually gets set for C20 Phy which makes checking the ssc_enabled Useless, which I fix in the next patch. Would be great if you could also have a look at that. Regards, Suraj Kandpal > > > Bspec: 64568, 74165, 74489, 74491 > > Fixes: 237e7be0bf57 ("drm/i915/mtl: For DP2.0 10G and 20G rates use > > MPLLA") > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > index e768dc6a15b3..3fd959a2773c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > @@ -2747,7 +2747,7 @@ static void intel_program_port_clock_ctl(struct > intel_encoder *encoder, > > /* TODO: HDMI FRL */ > > /* DP2.0 10G and 20G rates enable MPLLA*/ > > if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == > 2000000) > > - val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? > XELPDP_SSC_ENABLE_PLLA : 0; > > + val |= XELPDP_SSC_ENABLE_PLLA; > > else > > val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? > > XELPDP_SSC_ENABLE_PLLB : 0; > > > > -- > > 2.34.1 > >
On Wed, Jan 08, 2025 at 08:04:58AM +0200, Kandpal, Suraj wrote: > > > > -----Original Message----- > > From: Deak, Imre <imre.deak@intel.com> > > Sent: Tuesday, January 7, 2025 8:33 PM > > To: Kandpal, Suraj <suraj.kandpal@intel.com> > > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Nautiyal, > > Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma > > <uma.shankar@intel.com> > > Subject: Re: [PATCH 1/2] drm/i915/cx0: Fix SSC enablement in > > PORT_CLOCK_CTL > > > > On Mon, Jan 06, 2025 at 09:38:20AM +0530, Suraj Kandpal wrote: > > > SSC for PLL_A is enabled for UHBR10 or UHBR20 regardless of the need > > > for SSC. This means the ssc_enabled variable had no say to determine > > > enablement of SSC on PLL A. > > > > I don't see the above in the spec. It suggests that SSC should be enabled on > > PLL A for MFD, but in any case SSC can only be enabled if the sink supports > > SSC, as indicated by dpll_hw_state.cx0pll.ssc_enabled. > > Hi Imre, > > You are right In Bspec 74489 under Non-thunderbolt PLL Enable Sequence > It says SSC enable to be done on PLLA for MFD when on UHBR10 or > UHBR20 (PLLA is only used for C20 PHY UHBR10 and 20.) and check for > ssc_enabled for Native mode to enable SSC but now the issue is that we > aren't checking for MFD mode any particular reason for this ? and how > would we go about checking if we are in MFD mode or not ? Also the > ssc_enabled bool variable never actually gets set for C20 Phy which > makes checking the ssc_enabled Useless, which I fix in the next patch. Enabling SSC for DP outputs would require more changes than enabling SSC in the PLL, AFAICS at least: - Check the sink if it supports SSC. - Check VBT if it requires SSC to be enabled. - If enabling SSC, also enable it in the sink's spread control DPCD register. - If enabling SSC, adjust the MST BW calculation. In fact I'm not sure how this works atm on the DG2 SNPS and C10 PHYs, where SSC is enabled w/o checking/handling all the above. > Would be great if you could also have a look at that. > > Regards, > Suraj Kandpal > > > > > > Bspec: 64568, 74165, 74489, 74491 > > > Fixes: 237e7be0bf57 ("drm/i915/mtl: For DP2.0 10G and 20G rates use > > > MPLLA") > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > > index e768dc6a15b3..3fd959a2773c 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > > > @@ -2747,7 +2747,7 @@ static void intel_program_port_clock_ctl(struct > > intel_encoder *encoder, > > > /* TODO: HDMI FRL */ > > > /* DP2.0 10G and 20G rates enable MPLLA*/ > > > if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == > > 2000000) > > > - val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? > > XELPDP_SSC_ENABLE_PLLA : 0; > > > + val |= XELPDP_SSC_ENABLE_PLLA; > > > else > > > val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? > > > XELPDP_SSC_ENABLE_PLLB : 0; > > > > > > -- > > > 2.34.1 > > >
diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c index e768dc6a15b3..3fd959a2773c 100644 --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c @@ -2747,7 +2747,7 @@ static void intel_program_port_clock_ctl(struct intel_encoder *encoder, /* TODO: HDMI FRL */ /* DP2.0 10G and 20G rates enable MPLLA*/ if (crtc_state->port_clock == 1000000 || crtc_state->port_clock == 2000000) - val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLA : 0; + val |= XELPDP_SSC_ENABLE_PLLA; else val |= crtc_state->dpll_hw_state.cx0pll.ssc_enabled ? XELPDP_SSC_ENABLE_PLLB : 0;
SSC for PLL_A is enabled for UHBR10 or UHBR20 regardless of the need for SSC. This means the ssc_enabled variable had no say to determine enablement of SSC on PLL A. Bspec: 64568, 74165, 74489, 74491 Fixes: 237e7be0bf57 ("drm/i915/mtl: For DP2.0 10G and 20G rates use MPLLA") Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/intel_cx0_phy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)