| Message ID | 20190911133129.27466-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915: Fix cdclk bypass freq readout for tgl/bxt/glk | expand |
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 >
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
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.
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
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
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);