[2/4] drm/i915: Fix CD2X pipe select masking during cdclk sanitation
diff mbox series

Message ID 20190911133129.27466-2-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • [1/4] drm/i915: Fix cdclk bypass freq readout for tgl/bxt/glk
Related show

Commit Message

Ville Syrjälä Sept. 11, 2019, 1:31 p.m. UTC
From: Ville Syrjälä <ville.syrjala@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@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@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 42 ++++++++++++----------
 1 file changed, 23 insertions(+), 19 deletions(-)

Comments

Matt Roper Sept. 11, 2019, 3 p.m. UTC | #1
On Wed, Sep 11, 2019 at 04:31:27PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@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@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I'm confused why pre-merge CI flagged the results as a success if TGL
was hitting this?


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
>
Chris Wilson Sept. 11, 2019, 3:08 p.m. UTC | #2
Quoting Matt Roper (2019-09-11 16:00:44)
> I'm confused why pre-merge CI flagged the results as a success if TGL
> was hitting this?

We've only just (Fri) got CI's Tigerlake surviving boot, so a second
failure during boot would have been missed. For the summary report,
Tigerlake is currently suppressed as it is not yet proven itself as
being a stable result. (Which is a bit of a nuisance as you have to
remind yourself to actually check the details.)
-Chris
Jani Nikula Sept. 11, 2019, 3:10 p.m. UTC | #3
On Wed, 11 Sep 2019, Matt Roper <matthew.d.roper@intel.com> wrote:
> I'm confused why pre-merge CI flagged the results as a success if TGL
> was hitting this?

I didn't check the specifics, but the full set of IGT tests is only run
on a limited number of platforms, and TGL is not yet one of them. You
get the narrow range of tests on a wide range of platforms and the wide
range of tests on a narrow range of platforms.

BR,
Jani.
Saarinen, Jani Sept. 11, 2019, 7:02 p.m. UTC | #4
HI, 

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Matt
> Roper
> Sent: keskiviikko 11. syyskuuta 2019 18.01
> To: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: intel-gfx@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@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@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@linux.intel.com>
> 
> Reviewed-by: Matt Roper <matthew.d.roper@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Saarinen, Jani Sept. 11, 2019, 7:03 p.m. UTC | #5
HI, 

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Chris
> Wilson
> Sent: keskiviikko 11. syyskuuta 2019 18.09
> To: Roper, Matthew D <matthew.d.roper@intel.com>; Ville Syrjala
> <ville.syrjala@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix CD2X pipe select masking during
> cdclk sanitation
> 
> Quoting Matt Roper (2019-09-11 16:00:44)
> > I'm confused why pre-merge CI flagged the results as a success if TGL
> > was hitting this?
> 
> We've only just (Fri) got CI's Tigerlake surviving boot, so a second failure during boot
> would have been missed. For the summary report, Tigerlake is currently suppressed
> as it is not yet proven itself as being a stable result. (Which is a bit of a nuisance as
> you have to remind yourself to actually check the details.) -Chris
Exactly. 

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox series

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);