[Mesa-dev] [PATCH 1/2] i965: Correct a mistake that always forced texture tiling.

[Matt Turner]

On Wed, Aug 5, 2015 at 8:44 AM, Ben Widawsky <ben at 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 <mattst88 at 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.