Re: [PATCH] drm/i915/display: C20 clock state verification

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

On Fri, Dec 15, 2023 at 10:53:31AM +0200, Imre Deak wrote:
> On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
> > Add clock state verification for C20. Since we
> > are usign either A or B contexts, which are
> > selected based on clock rate, we first need to
> > calculate hw clock and use that clock to select
> > which context we are using.
> 
> Could the description be clearer that the patch _fixes_ the context
> selection? (Also the subject line should always say _what_ the patch
> does.)
> 
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 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 775c1c4a8978..6757e9f941e4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> >  	const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
> >  	bool use_mplla;
> >  	int i;
> > +	int hw_clock = intel_c20pll_calc_port_clock(encoder, mpll_hw_state);
> >  
> > -	use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
> > +	use_mplla = intel_c20_use_mplla(hw_clock);
> 
> It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW which
> context to use, so I think it's better to use the same condition here.

Yes, one should never assume anything about how the register values
were calculated/etc. That would pretty much defeat the whole purpose
of doing readout and state check.

BTW I don't see even being set anywhere C20_PHY_USE_MPLLB.
Do we not use it or is it encoded in that ugly hex soup in
intel_c20_compute_hdmi_tmds_pll()? Instead of repeating the
mistakes of the VLV PHY code someone should convert that into
human readable form...

> 
> >  	if (use_mplla) {
> >  		for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
> >  			I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
> > @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> >  				crtc->base.base.id, crtc->base.name, i,
> >  				mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
> >  	}
> > +
> > +	I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
> > +			"[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> > +			crtc->base.base.id, crtc->base.name,
> > +			mpll_sw_state->clock, hw_clock);
> 
> I think the intel_crtc_state::port_clock SW/HW state verification is
> equivalent to the above, but it's good to verify it here as well. I
> would store hw_clock to mpll_hw_state->clock in
> intel_c20pll_readout_hw_state() though.
> 
> >  }
> >  
> >  void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> > -- 
> > 2.34.1
> > 

-- 
Ville Syrjälä
Intel