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 <k...@bitplanet.net>
> > Signed-off-by: Ben Widawsky <b...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
