[Mesa-dev] [PATCH 2/2] i965/skl: [SQUASH] Update the MCS buffer scale sizes

[Ben Widawsky]

On Tue, Mar 10, 2015 at 09:43:26PM -0700, Kenneth Graunke wrote:
> On Thursday, February 26, 2015 03:42:53 PM Ben Widawsky wrote:
> > Keep this as a separate patch for review, but I will squash it with the previous
> > patch before pushing.
> > 
> > We don't support 16x MSAA yet, but I entered it in here while I was at the
> > table.
> > 
> > I'm having trouble getting through a piglit run on SKL at the moment, so I just
> > few a threw small tests at it:
> > 
> > tests/fast_color_clear/all-colors.shader_test
> > tests/fast_color_clear/non-redundant-clear.shader_test
> > tests/fast_color_clear/redundant-clear.shader_test
> > tests/fast_color_clear/fast-slow-clear-interaction.shader_test
> > 
> > Cc: Kristian Høgsberg <krh at bitplanet.net>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  src/gallium/drivers/ilo/ilo_layout.c            |  5 ++++-
> >  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c |  2 ++
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c   | 11 +++++++++--
> >  3 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/gallium/drivers/ilo/ilo_layout.c b/src/gallium/drivers/ilo/ilo_layout.c
> > index 0b639b2..c2c8ec5 100644
> > --- a/src/gallium/drivers/ilo/ilo_layout.c
> > +++ b/src/gallium/drivers/ilo/ilo_layout.c
> > @@ -1257,7 +1257,10 @@ layout_calculate_mcs_size(struct ilo_layout *layout,
> >           break;
> >        case INTEL_TILING_Y:
> >           downscale_x = 32 / layout->block_size;
> > -         downscale_y = 4;
> > +         if (brw->gen >= 9)
> > +            downscale_y = 2;
> > +         else
> > +            downscale_y = 4;
> >           break;
> >        default:
> >           assert(!"unsupported tiling mode");
> 
> Don't forget to drop this hunk.
> 
> > diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > index c4d4b68..f487e97 100644
> > --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > @@ -289,6 +289,8 @@ get_fast_clear_rect(struct brw_context *brw, struct gl_framebuffer *fb,
> >        case 8:
> >           x_scaledown = 2;
> >           break;
> > +      case 16:
> > +         x_scaledown = 1;
> >        default:
> >           unreachable("Unexpected sample count for fast clear");
> >        }
> 
> This hunk gets a R-b.  Searching for "width of clear rect" (in quotes)
> in the documentation finds the page with the table cited in the above
> comments...and it confirms your number here.
> 
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 36c3b26..75ce2b9 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -138,10 +138,17 @@ intel_get_non_msrt_mcs_alignment(struct brw_context *brw,
> >        unreachable("Non-MSRT MCS requires X or Y tiling");
> >        /* In release builds, fall through */
> >     case I915_TILING_Y:
> > -      *width_px = 32 / mt->cpp;
> > -      *height = 4;
> > +      if (brw->gen >= 9) {
> > +         const int scale_factor = 16 / mt->cpp;
> > +         *width_px = 128 / scale_factor;
> > +         *height = 64;
> > +      } else {
> > +         *width_px = 32 / mt->cpp;
> > +         *height = 4;
> > +      }
> 
> I'm not getting these numbers at all...the "Color Clear of
> Non-MultiSampler Render Target Restrictions" section still has the same
> table (TiledY RT CL/Pixels/Lines 32/8/4, 64/4/4, 128/2/4) cited in the
> comments above.  I see no Skylake changes here.
> 

Hmm. The able I am looking at is
32/128/64
64/64/64
128/32/64

It's the same page, but a separate table for SKL. Perhaps I did something wrong?

> We actually don't use non-MSRT MCS buffers on arrayed or mipmapped
> textures, IIRC - it's a new feature on Broadwell we never enabled (and
> probably should).  So the new SKL comments about alignment for those
> shouldn't matter just yet.

I wasn't considering this specifically, I just was looking at that table. If
that was not the table to look at, then yeah, the numbers wouldn't be right. I
can certainly try the old table and see if tests pass.

> 
> >        break;
> >     case I915_TILING_X:
> > +      assert(brw->gen < 9);
> 
> Adding the assert looks good to me, X-Tile is definitely not supported.
> 

I don't remember leaving this on purpose. I'd rather do this as a separate
patch. Thoughts?

> >        *width_px = 64 / mt->cpp;
> >        *height = 2;
> >     }
> > 
> 
> I think what you want instead is to edit brw_meta_fast_clear.c's
> get_fast_clear_rect() - which is where the other tables come into play.
> 
> You would change:
> 
> -      y_align *= 32;
> +      y_align *= brw->gen >= 9 ? 16 : 32;
> 
> ...
> -      y_scaledown = y_align / 2;
> +      y_scaledown = brw->gen >= 9 ? y_align : (y_align / 2);
> 
> 
> If you're actually just changing the get_fast_clear_rect patch, then
> squashing it into the previous patch seems appropriate after all.
> Either way's fine.

I'll take a look. I think we have to squash the patches though since the former
patch really doesn't accomplish anything without this, so any intermediate
piglit results are kind of useless. I can probably split out the MSAA, and X
tile assertion from this.

Thanks for looking.  Let me know if you can't find the table I am referring to.