[Intel-gfx] [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+
Paulo Zanoni
paulo.r.zanoni at intel.com
Tue Oct 23 00:12:18 UTC 2018
Em Seg, 2018-10-22 às 16:55 -0700, Rodrigo Vivi escreveu:
> On Mon, Oct 22, 2018 at 04:32:00PM -0700, Paulo Zanoni wrote:
> > Em Qui, 2018-10-18 às 16:14 +0300, Ville Syrjälä escreveu:
> > > On Tue, Oct 16, 2018 at 03:01:23PM -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.
> > > >
> > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > ---
> > > > 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 67a4d0735291..18157c6ee126 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -4696,28 +4696,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)) {
> > >
> > > IS_GEN9_BC || IS_BXT
> > >
> > > would be a little easier to parse, me thinks.
> >
> > I can do that, but what I really want is "DISPLAY_GEN(dev_priv) ==
> > 9".
>
> work in progress...
>
> btw...
>
> DISPLAY_GEN(dev_priv) == 9 or simply DISPLAY(dev_priv, 9) ?
It should mimic the model we already use: INTEL_GEN(dev_priv) >= 9.
I would expect a macro called DISPLAY() to return *the* Display or to
simply display somewhere the thing I pass as an argument. Now
DISPLAY_GEN() sounds more like it returns the GEN of the DISPLAY (or
generates a Display).
>
> I'm play around here with:
>
> #define DISPLAY(dev_priv, g) (!!((dev_priv)->info.display_mask &
> BIT(g-1)))
> #define DISPLAY_RANGE(dev_priv, s, e) \
> (!!((dev_priv)->info.display_mask &
> INTEL_GEN_MASK((s), (e))))
>
> thoughts? comments? ideas?
#define DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen)
>
> >
> > /me looks at Rodrigo
> >
> > >
> > > > + /* 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;
> > >
> > > This last thing is part of the glk+ watermark formula as well.
> > > But
> > > I'm not 100% convinced that it's needed.
> >
> > I simply can't find where this is documented. WAs 1125 and 1126,
> > which
> > contain text that match this code exactly, are not applicable to
> > GLK.
> > Which BSpec page and paragraph/section mentions this?
> >
> >
> > > One might assume that the the
> > > non-decrasing latency values guarantee that the resulting
> > > watermarks
> > > are also non-decreasing. But I've not actually done the math to
> > > see
> > > if
> > > that's true.
> > >
> > > Hmm. It might not actually be true on account of the 'memory
> > > latency
> > > microseconds >= line time microseconds' check when selecting
> > > which
> > > method to use to calculate the watermark. Not quite sure which
> > > way
> > > that would make things go.
> > >
> > > We also seem to be missing the res_lines handling here. But given
> > > that we only did this for compressed fbs before I don't think
> > > this
> > > patch is making things much worse by limiting this to pre-glk
> > > only.
> > > The glk+ handling and res_lines fix could be done as a followup.
> > >
> > > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > >
> > > > }
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 11) {
> > > > --
> > > > 2.14.4
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > >
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list