| Message ID | 20181004231600.14101-2-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Watermarks small fixes/improvements | expand |
On Thu, Oct 04, 2018 at 04:15:55PM -0700, Paulo Zanoni wrote: > BSpec does not show these WAs as applicable to GLK, and for CNL it > only shows them applicable for a super early pre-production stepping > we shouldn't be caring about anymore. Remove these so we can avoid > them on ICL too. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> From a quick grep, it looks like there's a couple other CNL A0-specific workarounds in the codebase (in the watermark code). If we don't want to handle CNL A0 in this patch, then I think we should first land a patch that removes the others and updates intel_detect_preproduction_hw() to make it explicit that this is no longer a supported stepping. > --- > drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1392aa56a55a..910551e04d16 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4687,28 +4687,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > res_lines = div_round_up_fixed16(selected_result, > wp->plane_blocks_per_line); > > - /* Display WA #1125: skl,bxt,kbl,glk */ > - if (level == 0 && wp->rc_surface) > - res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum); > + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { > + /* Display WA #1125: skl,bxt,kbl */ Although the code is right, should we just write "gen9" (or gen9display) in this comment (and the one for 1126)? That's how the bspec lists it (GEN9:All), and removes the ambiguity of whether "kbl" in this context is also supposed to cover CFL, AML, WHL (which it does). Matt > + if (level == 0 && wp->rc_surface) > + res_blocks += > + fixed16_to_u32_round_up(wp->y_tile_minimum); > + > + /* Display WA #1126: skl,bxt,kbl */ > + if (level >= 1 && level <= 7) { > + if (wp->y_tiled) { > + res_blocks += > + fixed16_to_u32_round_up(wp->y_tile_minimum); > + res_lines += wp->y_min_scanlines; > + } else { > + res_blocks++; > + } > > - /* Display WA #1126: skl,bxt,kbl,glk */ > - if (level >= 1 && level <= 7) { > - if (wp->y_tiled) { > - res_blocks += fixed16_to_u32_round_up( > - wp->y_tile_minimum); > - res_lines += wp->y_min_scanlines; > - } else { > - res_blocks++; > + /* > + * Make sure result blocks for higher latency levels are > + * atleast as high as level below the current level. > + * Assumption in DDB algorithm optimization for special > + * cases. Also covers Display WA #1125 for RC. > + */ > + if (result_prev->plane_res_b > res_blocks) > + res_blocks = result_prev->plane_res_b; > } > - > - /* > - * Make sure result blocks for higher latency levels are atleast > - * as high as level below the current level. > - * Assumption in DDB algorithm optimization for special cases. > - * Also covers Display WA #1125 for RC. > - */ > - if (result_prev->plane_res_b > res_blocks) > - res_blocks = result_prev->plane_res_b; > } > > if (INTEL_GEN(dev_priv) >= 11) { > -- > 2.14.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Ter, 2018-10-09 às 16:55 -0700, Matt Roper escreveu: > On Thu, Oct 04, 2018 at 04:15:55PM -0700, Paulo Zanoni wrote: > > BSpec does not show these WAs as applicable to GLK, and for CNL it > > only shows them applicable for a super early pre-production > > stepping > > we shouldn't be caring about anymore. Remove these so we can avoid > > them on ICL too. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > From a quick grep, it looks like there's a couple other CNL A0- > specific > workarounds in the codebase (in the watermark code). If we don't > want > to handle CNL A0 in this patch, then I think we should first land a > patch that removes the others and updates > intel_detect_preproduction_hw() to make it explicit that this is no > longer a supported stepping. This patch (and series) is potentially a "bug fix" due to changes in real-world not-A0 hardware (although I didn't mark this one as a fix since I doubt it will close anything in Bugzilla). The patch to remove the implementation of WAs in CNL:A0 is not a bug fix, but a rework. Fixes should always come before the reworks in order to facilitate backporting efforts. I do intend to propose patches removing the CNL A0 watermarks code (along with other reworks I have in mind), but I wanted to sort the bugs first. > > > --- > > drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++------- > > ------------ > > 1 file changed, 23 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 1392aa56a55a..910551e04d16 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4687,28 +4687,31 @@ static int skl_compute_plane_wm(const > > struct drm_i915_private *dev_priv, > > res_lines = div_round_up_fixed16(selected_result, > > wp- > > >plane_blocks_per_line); > > > > - /* Display WA #1125: skl,bxt,kbl,glk */ > > - if (level == 0 && wp->rc_surface) > > - res_blocks += fixed16_to_u32_round_up(wp- > > >y_tile_minimum); > > + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { > > + /* Display WA #1125: skl,bxt,kbl */ > > Although the code is right, should we just write "gen9" (or > gen9display) > in this comment (and the one for 1126)? That's how the bspec lists > it > (GEN9:All), and removes the ambiguity of whether "kbl" in this > context > is also supposed to cover CFL, AML, WHL (which it does). Although that makes sense, it doesn't seem to follow the current (undocumented?) standard we have for display WAs. I can totally do this, but I would like some more opinions first. CC'ing the maintainers. Thanks for the reviews, Paulo > > > Matt > > > + if (level == 0 && wp->rc_surface) > > + res_blocks += > > + fixed16_to_u32_round_up(wp- > > >y_tile_minimum); > > + > > + /* Display WA #1126: skl,bxt,kbl */ > > + if (level >= 1 && level <= 7) { > > + if (wp->y_tiled) { > > + res_blocks += > > + fixed16_to_u32_round_up(wp- > > >y_tile_minimum); > > + res_lines += wp->y_min_scanlines; > > + } else { > > + res_blocks++; > > + } > > > > - /* Display WA #1126: skl,bxt,kbl,glk */ > > - if (level >= 1 && level <= 7) { > > - if (wp->y_tiled) { > > - res_blocks += fixed16_to_u32_round_up( > > - wp- > > >y_tile_minimum); > > - res_lines += wp->y_min_scanlines; > > - } else { > > - res_blocks++; > > + /* > > + * Make sure result blocks for higher > > latency levels are > > + * atleast as high as level below the > > current level. > > + * Assumption in DDB algorithm > > optimization for special > > + * cases. Also covers Display WA #1125 for > > RC. > > + */ > > + if (result_prev->plane_res_b > res_blocks) > > + res_blocks = result_prev- > > >plane_res_b; > > } > > - > > - /* > > - * Make sure result blocks for higher latency > > levels are atleast > > - * as high as level below the current level. > > - * Assumption in DDB algorithm optimization for > > special cases. > > - * Also covers Display WA #1125 for RC. > > - */ > > - if (result_prev->plane_res_b > res_blocks) > > - res_blocks = result_prev->plane_res_b; > > } > > > > if (INTEL_GEN(dev_priv) >= 11) { > > -- > > 2.14.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 1392aa56a55a..910551e04d16 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4687,28 +4687,31 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, res_lines = div_round_up_fixed16(selected_result, wp->plane_blocks_per_line); - /* Display WA #1125: skl,bxt,kbl,glk */ - if (level == 0 && wp->rc_surface) - res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum); + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { + /* Display WA #1125: skl,bxt,kbl */ + if (level == 0 && wp->rc_surface) + res_blocks += + fixed16_to_u32_round_up(wp->y_tile_minimum); + + /* Display WA #1126: skl,bxt,kbl */ + if (level >= 1 && level <= 7) { + if (wp->y_tiled) { + res_blocks += + fixed16_to_u32_round_up(wp->y_tile_minimum); + res_lines += wp->y_min_scanlines; + } else { + res_blocks++; + } - /* Display WA #1126: skl,bxt,kbl,glk */ - if (level >= 1 && level <= 7) { - if (wp->y_tiled) { - res_blocks += fixed16_to_u32_round_up( - wp->y_tile_minimum); - res_lines += wp->y_min_scanlines; - } else { - res_blocks++; + /* + * Make sure result blocks for higher latency levels are + * atleast as high as level below the current level. + * Assumption in DDB algorithm optimization for special + * cases. Also covers Display WA #1125 for RC. + */ + if (result_prev->plane_res_b > res_blocks) + res_blocks = result_prev->plane_res_b; } - - /* - * Make sure result blocks for higher latency levels are atleast - * as high as level below the current level. - * Assumption in DDB algorithm optimization for special cases. - * Also covers Display WA #1125 for RC. - */ - if (result_prev->plane_res_b > res_blocks) - res_blocks = result_prev->plane_res_b; } if (INTEL_GEN(dev_priv) >= 11) {
BSpec does not show these WAs as applicable to GLK, and for CNL it only shows them applicable for a super early pre-production stepping we shouldn't be caring about anymore. Remove these so we can avoid them on ICL too. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 20 deletions(-)