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.ro...@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zan...@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".

/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.syrj...@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

Reply via email to