On Thu, Sep 22, 2022 at 02:56:53PM +0300, Jani Nikula wrote:
> On Thu, 22 Sep 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Fixes: b000abd3b3d2 ("drm/i915: Do .crtc_compute_clock() earlier")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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
