diff mbox series

[1/4] drm/i915: Force DPLL calculation for TC ports after readout

Message ID 20220921211525.10675-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Fix TC port PLLs after readout | expand

Commit Message

Ville Syrjälä Sept. 21, 2022, 9:15 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We always allocate two DPLLs (TC and TBT) for TC ports. This
is because we can't know ahead of time wherher we need to put
the PHY into DP-Alt or TBT mode.

However during readout we can obviously only read out the state
of the DPLL that the port is actually using. Thus the state after
readout will not have both DPLLs populated.

We run into problems if during readout the TC port is in DP-Alt
mode, but we then perform a modeset on the port without going
through the full .compute_config() machinery, and during said
modeset the port cannot be switched back into DP-Alt mode and
we need to take the TBT fallback path. Such a modeset can
happen eg. due to cdclk reprogramming.

This wasn't a problem earlier because we did all the DPLL
calculations much later in the modeset. So even if flagged
a modeset very late we'd still have gone through the DPLL
calculations. But now all the DPLL calculations happen much
earlier and so we need to deal with it, or else we'll attempt
a modeset without a DPLL.

To guarantee that we always have both DPLLs fully cal/ulated
for TC ports force a full modeset computation during the
initial commit.

Reported-by: Lee Shawn C <shawn.c.lee@intel.com>
Fixes: b000abd3b3d2 ("drm/i915: Do .crtc_compute_clock() earlier")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Jani Nikula Sept. 22, 2022, 11:56 a.m. UTC | #1 
On Thu, 22 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We always allocate two DPLLs (TC and TBT) for TC ports. This
> is because we can't know ahead of time wherher we need to put
> the PHY into DP-Alt or TBT mode.
>
> However during readout we can obviously only read out the state
> of the DPLL that the port is actually using. Thus the state after
> readout will not have both DPLLs populated.
>
> We run into problems if during readout the TC port is in DP-Alt
> mode, but we then perform a modeset on the port without going
> through the full .compute_config() machinery, and during said
> modeset the port cannot be switched back into DP-Alt mode and
> we need to take the TBT fallback path. Such a modeset can
> happen eg. due to cdclk reprogramming.
>
> This wasn't a problem earlier because we did all the DPLL
> calculations much later in the modeset. So even if flagged
> a modeset very late we'd still have gone through the DPLL
> calculations. But now all the DPLL calculations happen much
> earlier and so we need to deal with it, or else we'll attempt
> a modeset without a DPLL.
>
> To guarantee that we always have both DPLLs fully cal/ulated
> for TC ports force a full modeset computation during the
> initial commit.
>
> Reported-by: Lee Shawn C <shawn.c.lee@intel.com>
> Fixes: b000abd3b3d2 ("drm/i915: Do .crtc_compute_clock() earlier")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 643832d55c28..6278b8ea5bf1 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3600,10 +3600,21 @@ static void intel_ddi_sync_state(struct intel_encoder *encoder,
>  static bool intel_ddi_initial_fastset_check(struct intel_encoder *encoder,
>  					    struct intel_crtc_state *crtc_state)
>  {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum phy phy = intel_port_to_phy(i915, encoder->port);
> +	bool ret = true;
> +
> +	if (intel_phy_is_tc(i915, phy)) {
> +		drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset to compute TC port DPLLs\n",
> +			    encoder->base.base.id, encoder->base.name);
> +		crtc_state->uapi.mode_changed = true;
> +		ret = false;
> +	}
> +
>  	if (intel_crtc_has_dp_encoder(crtc_state))
> -		return intel_dp_initial_fastset_check(encoder, crtc_state);
> +		ret &= intel_dp_initial_fastset_check(encoder, crtc_state);

I think there have been static checker warnings about mixing bitwise and
boolean AND like this. I guess there's implicit type conversion to int
and back to bool.

BR,
Jani.

>  
> -	return true;
> +	return ret;
>  }
>  
>  static enum intel_output_type
Ville Syrjälä Sept. 22, 2022, 3:40 p.m. UTC | #2 
On Thu, Sep 22, 2022 at 02:56:53PM +0300, Jani Nikula wrote:
> On Thu, 22 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We always allocate two DPLLs (TC and TBT) for TC ports. This
> > is because we can't know ahead of time wherher we need to put
> > the PHY into DP-Alt or TBT mode.
> >
> > However during readout we can obviously only read out the state
> > of the DPLL that the port is actually using. Thus the state after
> > readout will not have both DPLLs populated.
> >
> > We run into problems if during readout the TC port is in DP-Alt
> > mode, but we then perform a modeset on the port without going
> > through the full .compute_config() machinery, and during said
> > modeset the port cannot be switched back into DP-Alt mode and
> > we need to take the TBT fallback path. Such a modeset can
> > happen eg. due to cdclk reprogramming.
> >
> > This wasn't a problem earlier because we did all the DPLL
> > calculations much later in the modeset. So even if flagged
> > a modeset very late we'd still have gone through the DPLL
> > calculations. But now all the DPLL calculations happen much
> > earlier and so we need to deal with it, or else we'll attempt
> > a modeset without a DPLL.
> >
> > To guarantee that we always have both DPLLs fully cal/ulated
> > for TC ports force a full modeset computation during the
> > initial commit.
> >
> > Reported-by: Lee Shawn C <shawn.c.lee@intel.com>
> > Fixes: b000abd3b3d2 ("drm/i915: Do .crtc_compute_clock() earlier")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 643832d55c28..6278b8ea5bf1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3600,10 +3600,21 @@ static void intel_ddi_sync_state(struct intel_encoder *encoder,
> >  static bool intel_ddi_initial_fastset_check(struct intel_encoder *encoder,
> >  					    struct intel_crtc_state *crtc_state)
> >  {
> > +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > +	enum phy phy = intel_port_to_phy(i915, encoder->port);
> > +	bool ret = true;
> > +
> > +	if (intel_phy_is_tc(i915, phy)) {
> > +		drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset to compute TC port DPLLs\n",
> > +			    encoder->base.base.id, encoder->base.name);
> > +		crtc_state->uapi.mode_changed = true;
> > +		ret = false;
> > +	}
> > +
> >  	if (intel_crtc_has_dp_encoder(crtc_state))
> > -		return intel_dp_initial_fastset_check(encoder, crtc_state);
> > +		ret &= intel_dp_initial_fastset_check(encoder, crtc_state);
> 
> I think there have been static checker warnings about mixing bitwise and
> boolean AND like this. I guess there's implicit type conversion to int
> and back to bool.

I guess I could rewite it something like

if (intel_crtc_has_dp_encoder() &&
    !intel_dp_initial_fastset_check())
	ret = false;

Hmm. Maybe I should also use a better name than 'ret'.
The true vs. false polarity used here makes that a bit confusing
to me. So calling it 'fastset' or something would probably be better.

> 
> BR,
> Jani.
> 
> >  
> > -	return true;
> > +	return ret;
> >  }
> >  
> >  static enum intel_output_type
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 643832d55c28..6278b8ea5bf1 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3600,10 +3600,21 @@  static void intel_ddi_sync_state(struct intel_encoder *encoder,
 static bool intel_ddi_initial_fastset_check(struct intel_encoder *encoder,
 					    struct intel_crtc_state *crtc_state)
 {
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+	enum phy phy = intel_port_to_phy(i915, encoder->port);
+	bool ret = true;
+
+	if (intel_phy_is_tc(i915, phy)) {
+		drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset to compute TC port DPLLs\n",
+			    encoder->base.base.id, encoder->base.name);
+		crtc_state->uapi.mode_changed = true;
+		ret = false;
+	}
+
 	if (intel_crtc_has_dp_encoder(crtc_state))
-		return intel_dp_initial_fastset_check(encoder, crtc_state);
+		ret &= intel_dp_initial_fastset_check(encoder, crtc_state);
 
-	return true;
+	return ret;
 }
 
 static enum intel_output_type