Re: [Intel-gfx] [PATCH 1/4] drm/i915: Force DPLL calculation for TC ports after readout

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

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

-- 
Ville Syrjälä
Intel