On Wed, Aug 5, 2015 at 8:44 AM, Ben Widawsky <b...@bwidawsk.net> wrote: > On Tue, Aug 04, 2015 at 11:16:55PM -0700, Matt Turner wrote: >> On Tue, Aug 4, 2015 at 11:18 PM, Matt Turner <matts...@gmail.com> wrote: >> > Regression since commit 3a31876600, when tiling modes were moved into >> > layout_flags. >> > > It be interesting to find out why this caused the perf regression.
I think it's because tiling small textures is bad and we were forcing a tiling on everything. >> > The relevant enum values are >> > >> > MIPTREE_LAYOUT_ALLOC_YTILED = 1 << 5 >> > MIPTREE_LAYOUT_ALLOC_XTILED = 1 << 6 >> > MIPTREE_LAYOUT_ALLOC_ANY_TILED = MIPTREE_LAYOUT_ALLOC_YTILED | >> > MIPTREE_LAYOUT_ALLOC_XTILED >> > MIPTREE_LAYOUT_ALLOC_LINEAR = 1 << 7 >> > >> > so the expression (layout_flags & MIPTREE_LAYOUT_ALLOC_ANY_TILED) can >> > never produce a value of MIPTREE_LAYOUT_ALLOC_LINEAR. >> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91513 >> > --- >> > I find this code really weird. MIPTREE_LAYOUT_ALLOC_XTILED is only used >> > in the two places noted in the commit message, and it wasn't an enum >> > value in the intel_miptree_tiling_mode type removed in the aforementioned >> > commit. Even with this patch, the brw_miptree_choose_tiling() function >> > wouldn't do anything sensible if passed XTILED | ~YTILED in layout_flags. >> > In fact, that case isn't even handled by the switch statement... >> > > > I could imagine that being a problem pre-gen6, but I think once we had > Y-tiling > and blitter support, we never actually use X-tiled except for the scanout > buffer. /me shrugs. > >> > src/mesa/drivers/dri/i965/brw_tex_layout.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > w >> > diff --git a/src/m esa/drivers/dri/i965/brw_tex_layout.c >> > b/src/mesa/drivers/dri/i965/brw_tex_layout.c >> > index fb78b08..8f492e3 100644 >> > --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c >> > +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c >> > @@ -633,11 +633,11 @@ brw_miptree_choose_tiling(struct brw_context *brw, >> > switch (layout_flags & MIPTREE_LAYOUT_ALLOC_ANY_TILED) { >> > case MIPTREE_LAYOUT_ALLOC_ANY_TILED: >> > break; >> > case MIPTREE_LAYOUT_ALLOC_YTILED: >> > return I915_TILING_Y; >> > - case MIPTREE_LAYOUT_ALLOC_LINEAR: >> > + case 0: >> >> Maybe a better fix is to include LINEAR in the definition of ANY? I >> suspect that matches the previous behavior better than this. >> > > Hmm. Looking at the part you change it looks like what I meant. I do want any > to > mean any type of tiling, Y, X, eventually Yf, and Ys. What about: > > switch (layout_flags & (MIPTREE_LAYOUT_ALLOC_ANY_TILED | LINEAR)) Actually, the enum you removed was enum intel_miptree_tiling_mode { INTEL_MIPTREE_TILING_ANY, INTEL_MIPTREE_TILING_Y, INTEL_MIPTREE_TILING_NONE, }; so I think "ANY" really means "Y" or "NONE" (i.e., linear). I'll send a new patch. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
