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

[Jani Nikula <jani.nikula@intel.com>]

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. Does the display gen
adequately cover the differences between platforms ultimately?

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. 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.

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