[Intel-gfx] [PATCH 2/4] drm/i915: Fix CD2X pipe select masking during cdclk sanitation

[Saarinen, Jani]

HI, 

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Matt
> Roper
> Sent: keskiviikko 11. syyskuuta 2019 18.01
> To: Ville Syrjala <ville.syrjala at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix CD2X pipe select masking during
> cdclk sanitation
> 
> On Wed, Sep 11, 2019 at 04:31:27PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > We're forgetting to mask off all three pipe select bits from the
> > CDCLK_CTL value on icl+ which may lead to the extra bit being left in.
> > That will cause us to consider the current hardware cdclk state as
> > invalid, and we proceed to sanitize it even though the hardware may
> > have active pipes and whatnot.
> >
> > Fix up the mask so we get rid of all three pipe select bits and thus
> > hopefully no longer sanitize cdclk when it's already correctly
> > programmed.
> >
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111641
> > Fixes: 0c1279b58fc7 ("drm/i915: Consolidate {bxt,cnl,icl}_init_cdclk")
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> 
> I'm confused why pre-merge CI flagged the results as a success if TGL was hitting
> this?
It died also on CI: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14345/?
Martin, Tomi can you explain? 

> 
> 
> Matt
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 42
> > ++++++++++++----------
> >  1 file changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 6b75d2a91cd9..f59a6f775177 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1464,6 +1464,26 @@ static void cnl_cdclk_pll_enable(struct
> drm_i915_private *dev_priv, int vco)
> >  	dev_priv->cdclk.hw.vco = vco;
> >  }
> >
> > +static u32 bxt_cdclk_cd2x_pipe(struct drm_i915_private *dev_priv,
> > +enum pipe pipe) {
> > +	if (INTEL_GEN(dev_priv) >= 12) {
> > +		if (pipe == INVALID_PIPE)
> > +			return TGL_CDCLK_CD2X_PIPE_NONE;
> > +		else
> > +			return TGL_CDCLK_CD2X_PIPE(pipe);
> > +	} else if (INTEL_GEN(dev_priv) >= 11) {
> > +		if (pipe == INVALID_PIPE)
> > +			return ICL_CDCLK_CD2X_PIPE_NONE;
> > +		else
> > +			return ICL_CDCLK_CD2X_PIPE(pipe);
> > +	} else {
> > +		if (pipe == INVALID_PIPE)
> > +			return BXT_CDCLK_CD2X_PIPE_NONE;
> > +		else
> > +			return BXT_CDCLK_CD2X_PIPE(pipe);
> > +	}
> > +}
> > +
> >  static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_state *cdclk_state,
> >  			  enum pipe pipe)
> > @@ -1534,24 +1554,8 @@ static void bxt_set_cdclk(struct drm_i915_private
> *dev_priv,
> >  			bxt_de_pll_enable(dev_priv, vco);
> >  	}
> >
> > -	val = divider | skl_cdclk_decimal(cdclk);
> > -
> > -	if (INTEL_GEN(dev_priv) >= 12) {
> > -		if (pipe == INVALID_PIPE)
> > -			val |= TGL_CDCLK_CD2X_PIPE_NONE;
> > -		else
> > -			val |= TGL_CDCLK_CD2X_PIPE(pipe);
> > -	} else if (INTEL_GEN(dev_priv) >= 11) {
> > -		if (pipe == INVALID_PIPE)
> > -			val |= ICL_CDCLK_CD2X_PIPE_NONE;
> > -		else
> > -			val |= ICL_CDCLK_CD2X_PIPE(pipe);
> > -	} else {
> > -		if (pipe == INVALID_PIPE)
> > -			val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > -		else
> > -			val |= BXT_CDCLK_CD2X_PIPE(pipe);
> > -	}
> > +	val = divider | skl_cdclk_decimal(cdclk) |
> > +		bxt_cdclk_cd2x_pipe(dev_priv, pipe);
> >
> >  	/*
> >  	 * Disable SSA Precharge when CD clock frequency < 500 MHz, @@
> > -1620,7 +1624,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private
> *dev_priv)
> >  	 * dividers both synching to an active pipe, or asynchronously
> >  	 * (PIPE_NONE).
> >  	 */
> > -	cdctl &= ~BXT_CDCLK_CD2X_PIPE_NONE;
> > +	cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> >
> >  	/* Make sure this is a legal cdclk value for the platform */
> >  	cdclk = bxt_calc_cdclk(dev_priv, dev_priv->cdclk.hw.cdclk);
> > --
> > 2.21.0
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx