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

[Matt Turner]

On Wed, Aug 5, 2015 at 5:12 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Wed, Aug 05, 2015 at 12:08:31PM -0700, Matt Turner wrote:
>> Regression since commit 3a31876600, when tiling modes were moved into
>> layout_flags.
>>
>> 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.
>>
>> The enum this replaced was
>>
>>    enum intel_miptree_tiling_mode {
>>       INTEL_MIPTREE_TILING_ANY,
>>       INTEL_MIPTREE_TILING_Y,
>>       INTEL_MIPTREE_TILING_NONE,
>>    };
>>
>> where "ANY" means "Y" or "NONE" (i.e., linear). As such, remove the
>> unused (and worse, unhandled) MIPTREE_LAYOUT_ALLOC_XTILED and redefine
>> MIPTREE_LAYOUT_ALLOC_ANY_TILED to mean what it did before.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91513
>> ---
>>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
>> index 89fdccb..506c258 100644
>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
>> @@ -537,13 +537,11 @@ enum {
>>     MIPTREE_LAYOUT_FORCE_HALIGN16           = 1 << 4,
>>
>>     MIPTREE_LAYOUT_ALLOC_YTILED             = 1 << 5,
>> -   MIPTREE_LAYOUT_ALLOC_XTILED             = 1 << 6,
>> -   MIPTREE_LAYOUT_ALLOC_LINEAR             = 1 << 7,
>> +   MIPTREE_LAYOUT_ALLOC_LINEAR             = 1 << 6,
>> +   MIPTREE_LAYOUT_ALLOC_ANY_TILED          = MIPTREE_LAYOUT_ALLOC_YTILED |
>> +                                             MIPTREE_LAYOUT_ALLOC_LINEAR,
>>  };
>>
>> -#define MIPTREE_LAYOUT_ALLOC_ANY_TILED (MIPTREE_LAYOUT_ALLOC_YTILED | \
>> -                                        MIPTREE_LAYOUT_ALLOC_XTILED)
>> -
>>  struct intel_mipmap_tree *intel_miptree_create(struct brw_context *brw,
>>                                                 GLenum target,
>>                                              mesa_format format,
>>
>
> This is going further in the wrong direction in my opinion (it mimics the old
> code more closely, but that that wasn't my original goal).

It would probably help me to understand what the goal actually is (I
see the comments about the goals in the commit summary, but it's kind
of nebulous). I see that we're merging the intel_miptree_tiling_mode
enum into layout_flags.

I suspect you're laying the foundation for Ys/Yf tilings?  Why do we
need tiling parameters as a bitfield? Are we going to want to ask for
a Y of Yf tiled miptree and let brw_miptree_choose_tiling() pick?

> "LINEAR" is not a
> tiled type. However, it fixes the bug, and you like it this way, so just do:
>
> s/MIPTREE_LAYOUT_ALLOC_ANY_TILED/MIPTREE_LAYOUT_ALLOC_ANY/
>
> and it's
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

Ignoring for a moment the discussion about the goals of your changes,
I think the original code is pretty clear that
INTEL_MIPTREE_TILING_ANY includes no tiling at all.

Going beyond the switch statement in brw_miptree_choose_tiling()
required that requested=INTEL_MIPTREE_TILING_ANY. From there depending
on the conditions the function would return X, Y, or NONE.

Actually, if I'm reading the diff correctly, there's another bug here:

    map->mt = intel_miptree_create(brw, GL_TEXTURE_2D, mt->format,
                                   0, 0,
                                   map->w, map->h, 1,
-                                  0, INTEL_MIPTREE_TILING_NONE, 0);
+                                  0, 0);

Shouldn't the last argument (layout_flags) have been
MIPTREE_LAYOUT_ALLOC_LINEAR? I think the bug I was fixing in this
patch would have masked it.