Re: [PATCH 01/11] drm/i915: don't apply Display WAs 1125 and 1126 to GLK/CNL+

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Oct 23, 2018 at 10:30:18AM +0300, Jani Nikula wrote:
> On Mon, 22 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Mon, Oct 22, 2018 at 05:12:18PM -0700, Paulo Zanoni wrote:
> >> 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@intel.com>
> >> > > > > 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(-)
> >> > > > > 
> >> > > > > 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.
> >
> > there's a macro defined on gen we end up never using
> > IS_GEN(9, GEN_FOREVER) with the same effect of INTEL_GEN(dev_priv) >= 9
> >
> > Should we just kill that or try to use more that instead of direct comparison?
> >
> > The advantage seems to be the use of bitmasks...
> >
> >> 
> >> 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).
> >
> > what about IS_DISPLAY(dev_priv, 9) ?
> > and IS_DISPLAY_RANGE(dev_priv, 5, 9)
> 
> Perhaps IS_DISPLAY_GEN(dev_priv, start, end).
> 
> *However* the naming and composition of the macro is *much* less
> important than what the code ends up looking.

I fully agree. For this reason I started with the already existent GEN checks.

> Does the display gen
> adequately cover the differences between platforms ultimately?

This thought got me starting many different attempts and then
using git reset --hard HEAD
so many times this week.

No, this is not enough to cover all the needs. Geminilake right
now is the only exception case. We could have another case on internal
right now where we could use same approach to make it better.

But still the ugliest part can be the intersaction and gray areas with
GEM part.

> 
> For example, a clear counter indication is that you'll be hard pressed
> to express the current HAS_GMCH_DISPLAY() in terms of display gen in a
> way that doesn't have to check for VLV/CHV.

Yeap, I don't think this display_gen solve this case. Although maybe
the range can help a bit to reduce the gmch_display checks.

> I don't think you can avoid
> IS_GEMINILAKE() either, you'll end up using display gen 10 in some
> places but Geminilake in others, ultimately making the end result worse
> than the starting point.

Well... I decided to give a try starting for the glk case to see
how it gets.

https://github.com/vivijim/drm-intel/tree/display_gen

Could you please take a look and let me know what do you think?

This version is taking display_gen to extreme, but we could
have a reduced one with glk checks inside bxt and dsi functions.

well, either way one fact is that we already have a code that is mixed with
IS_GEN and platform codename checks. Even if we still have some cases
where platform codename checks is unavoidable maybe it is better if we
prefer gen or display gen over platform codename when possible.

At least it reduces the mixed cases and gets easier to add
new platforms.

Thanks,
Rodrigo.

> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> 
> >> > 
> >> > 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@linux.intel.com>
> >> > > > 
> >> > > > 
> >> > > > >  	}
> >> > > > > 
> >> > > > >  	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
> >> > > > 
> >> > > > 
> >> > 
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx