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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx