diff mbox series

drm/i915/display: Calculate crtc clock rate based on PLL parameters

Message ID 20240502131716.504616-1-mika.kahola@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/display: Calculate crtc clock rate based on PLL parameters | expand

Commit Message

Mika Kahola May 2, 2024, 1:17 p.m. UTC
With HDMI monitors we bumped up a case where the crtc clock rate
caused a mismatch on state verification. This was due to
assumption that the SW clock rate from PLL structure would match
the calculated counterpart from HW. This is not necessarily always
the case and therefore we would actually need to recalculate the
clock rate from SW PLL parameters. Then these SW and HW crtc clock
rates can be compared with each other.

The patch recalculates the crtc clock rate for SW state based on
SW PLL parameters and compares the crtc clock rate calculated
from the parameters found from the HW.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Imre Deak May 3, 2024, 11 a.m. UTC | #1
On Thu, May 02, 2024 at 04:17:16PM +0300, Mika Kahola wrote:
> With HDMI monitors we bumped up a case where the crtc clock rate
> caused a mismatch on state verification. This was due to
> assumption that the SW clock rate from PLL structure would match
> the calculated counterpart from HW. This is not necessarily always
> the case and therefore we would actually need to recalculate the
> clock rate from SW PLL parameters. Then these SW and HW crtc clock
> rates can be compared with each other.
> 
> The patch recalculates the crtc clock rate for SW state based on
> SW PLL parameters and compares the crtc clock rate calculated
> from the parameters found from the HW.
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 3 ++-
>  1 file changed, 2 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 8e3b13884bb8..89a195917179 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -3078,9 +3078,10 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
>  	const struct intel_c20pll_state *mpll_sw_state = &state->dpll_hw_state.cx0pll.c20;
>  	bool sw_use_mpllb = intel_c20phy_use_mpllb(mpll_sw_state);
>  	bool hw_use_mpllb = intel_c20phy_use_mpllb(mpll_hw_state);
> +	int clock = intel_c20pll_calc_port_clock(encoder, mpll_sw_state);
>  	int i;
>  
> -	I915_STATE_WARN(i915, mpll_hw_state->clock != mpll_sw_state->clock,
> +	I915_STATE_WARN(i915, mpll_hw_state->clock != clock,

There is a corresponding check already in the encoder state checker,
which is more approriate, since it compares the calculated PLL clock
against the - adjusted - crtc port clock and I think that's the only
place all other platforms check this. In any case the above check
looks correct:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  			"[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
>  			crtc->base.base.id, crtc->base.name,
>  			mpll_sw_state->clock, mpll_hw_state->clock);
> -- 
> 2.34.1
>
Mika Kahola May 6, 2024, 7:45 a.m. UTC | #2
> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Friday, May 3, 2024 2:00 PM
> To: Kahola, Mika <mika.kahola@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/display: Calculate crtc clock rate based on PLL parameters
> 
> On Thu, May 02, 2024 at 04:17:16PM +0300, Mika Kahola wrote:
> > With HDMI monitors we bumped up a case where the crtc clock rate
> > caused a mismatch on state verification. This was due to assumption
> > that the SW clock rate from PLL structure would match the calculated
> > counterpart from HW. This is not necessarily always the case and
> > therefore we would actually need to recalculate the clock rate from SW
> > PLL parameters. Then these SW and HW crtc clock rates can be compared
> > with each other.
> >
> > The patch recalculates the crtc clock rate for SW state based on SW
> > PLL parameters and compares the crtc clock rate calculated from the
> > parameters found from the HW.
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 3 ++-
> >  1 file changed, 2 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 8e3b13884bb8..89a195917179 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -3078,9 +3078,10 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> >  	const struct intel_c20pll_state *mpll_sw_state = &state->dpll_hw_state.cx0pll.c20;
> >  	bool sw_use_mpllb = intel_c20phy_use_mpllb(mpll_sw_state);
> >  	bool hw_use_mpllb = intel_c20phy_use_mpllb(mpll_hw_state);
> > +	int clock = intel_c20pll_calc_port_clock(encoder, mpll_sw_state);
> >  	int i;
> >
> > -	I915_STATE_WARN(i915, mpll_hw_state->clock != mpll_sw_state->clock,
> > +	I915_STATE_WARN(i915, mpll_hw_state->clock != clock,
> 
> There is a corresponding check already in the encoder state checker, which is more approriate, since it compares the calculated
> PLL clock against the - adjusted - crtc port clock and I think that's the only place all other platforms check this. In any case the
> above check looks correct:
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Patch pushed. Thanks for the review!

-Mika-
> 
> >  			"[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> >  			crtc->base.base.id, crtc->base.name,
> >  			mpll_sw_state->clock, mpll_hw_state->clock);
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 8e3b13884bb8..89a195917179 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -3078,9 +3078,10 @@  static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
 	const struct intel_c20pll_state *mpll_sw_state = &state->dpll_hw_state.cx0pll.c20;
 	bool sw_use_mpllb = intel_c20phy_use_mpllb(mpll_sw_state);
 	bool hw_use_mpllb = intel_c20phy_use_mpllb(mpll_hw_state);
+	int clock = intel_c20pll_calc_port_clock(encoder, mpll_sw_state);
 	int i;
 
-	I915_STATE_WARN(i915, mpll_hw_state->clock != mpll_sw_state->clock,
+	I915_STATE_WARN(i915, mpll_hw_state->clock != clock,
 			"[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
 			crtc->base.base.id, crtc->base.name,
 			mpll_sw_state->clock, mpll_hw_state->clock);