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

[Kahola, Mika]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Friday, December 15, 2023 5:10 PM
> To: Kahola, Mika <mika.kahola at intel.com>; Deak, Imre <imre.deak at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: RE: [PATCH] drm/i915/display: C20 clock state verification
> 
> On Fri, 15 Dec 2023, "Kahola, Mika" <mika.kahola at intel.com> wrote:
> >> -----Original Message-----
> >> From: Deak, Imre <imre.deak at intel.com>
> >> Sent: Friday, December 15, 2023 11:02 AM
> >> To: Kahola, Mika <mika.kahola at intel.com>;
> >> intel-gfx at lists.freedesktop.org
> >> Subject: Re: [PATCH] drm/i915/display: C20 clock state verification
> >>
> >> On Fri, Dec 15, 2023 at 10:53:36AM +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.)
> > OK, should I add the fixes tag as well? I will reword the commit message to better describe what's going on in this patch.
> >
> >> >
> >> > >
> >> > > Signed-off-by: Mika Kahola <mika.kahola at 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.
> >
> > Maybe I will ditch the use_mplla completely and go directly with
> >
> > if (mpll_hw_state->tx]0] & C20_PHY_USE_MPLLB) { .. }
> >
> > instead?
> 
> You should first verify that the hw and sw states for use_mplla agree.
> 
> If they don't, it doesn't matter which one you use.

Right, I will compare these two context selection and throw a note if these two doesn't match.

Thanks!

-Mika-
> 
> BR,
> Jani.
> 
> 
> >
> >>
> >> You could also add a check intel_c20_use_mplla(clock) matches the above flag.
> >>
> >> >
> >> > >  	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.
> > Well, clock is part of pll structure anyway, so it might as well be checked here too.
> >
> > I will store hw clock too in intel_c20pll_readout_hw_state()
> >
> > Thanks!
> > Mika
> >
> >> >
> >> > >  }
> >> > >
> >> > >  void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> >> > > --
> >> > > 2.34.1
> >> > >
> 
> --
> Jani Nikula, Intel