On Thu, Aug 6, 2015 at 11:14 AM, Ben Widawsky <b...@bwidawsk.net> wrote:
> On Thu, Aug 06, 2015 at 11:03:55AM -0700, Matt Turner wrote:
>> Ben suggested that I rename MIPTREE_LAYOUT_ALLOC_ANY_TILED since it
>> needed to include no tiling at all, but the name
>> MIPTREE_LAYOUT_ALLOC_ANY is pretty nondescriptive. We can avoid
>> confusion by replacing "ALLOC" with "TILING" in the identifiers.
>
> The only nit I have, and the primary reason I chose "ALLOC" was because the
> flag
> is only relevant when you're creating a miptree with a new BO. There are
> functions to import a BO into a miptree, and those should never have such
> flags
> set. I was just trying to make it very explicit that this flag is only valid
> when you are creating the BO.
Ahh, I see. I'd thought it just meant "how it's allocated", i.e. tiled.
I see that we already have assertions that (layout_flags &
MIPTREE_LAYOUT_ALLOC_ANY_TILED) == 0. That seems sufficient to me. :)
> If you can think of any better way to describe it, I'd be appreciative, but
> since
> you fixed the bug, I don't mind you doing it as you like.
>
>> ---
>> Once reviewed, I plan to land the patches in the following order
>>
>> i965: Request a miptree with no tiling intel_miptree_map_blit().
>> i965: Correct a mistake that always forced texture tiling.
>> i965: Rename MIPTREE_LAYOUT_ALLOC_* -> MIPTREE_LAYOUT_TILING_*.
>>
>> The ordering of the first two is particularly important, since the
>> bug fixed in the first was masked by the bug in the second, landing
>> them in the other order would expose the bug fixed in the first.
>>
>> src/mesa/drivers/dri/i965/brw_tex_layout.c | 8 ++++----
>> src/mesa/drivers/dri/i965/intel_fbo.c | 2 +-
>> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 16 ++++++++--------
>> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 8 ++++----
>> src/mesa/drivers/dri/i965/intel_tex.c | 2 +-
>> src/mesa/drivers/dri/i965/intel_tex_image.c | 2 +-
>> src/mesa/drivers/dri/i965/intel_tex_validate.c | 2 +-
>> 7 files changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> index fb78b08..c9ee526 100644
>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> @@ -630,12 +630,12 @@ brw_miptree_choose_tiling(struct brw_context *brw,
>> /* Some usages may want only one type of tiling, like depth miptrees (Y
>> * tiled), or temporary BOs for uploading data once (linear).
>> */
>> - switch (layout_flags & MIPTREE_LAYOUT_ALLOC_ANY_TILED) {
>> - case MIPTREE_LAYOUT_ALLOC_ANY_TILED:
>> + switch (layout_flags & MIPTREE_LAYOUT_TILING_ANY) {
>> + case MIPTREE_LAYOUT_TILING_ANY:
>> break;
>> - case MIPTREE_LAYOUT_ALLOC_YTILED:
>> + case MIPTREE_LAYOUT_TILING_Y:
>> return I915_TILING_Y;
>> - case MIPTREE_LAYOUT_ALLOC_LINEAR:
>> + case MIPTREE_LAYOUT_TILING_NONE:
>> return I915_TILING_NONE;
>> }
>
> Just sanity checking myself, but the r-b at the bottom is assuming this is the
> only functional change (it was all I could spot).
If there's a functional change here, there shouldn't be. I can't spot
it -- what do you see?
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c
>> b/src/mesa/drivers/dri/i965/intel_fbo.c
>> index 87ba624..72648b0 100644
>> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
>> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
>> @@ -1023,7 +1023,7 @@ intel_renderbuffer_move_to_temp(struct brw_context
>> *brw,
>> int width, height, depth;
>>
>> uint32_t layout_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD |
>> - MIPTREE_LAYOUT_ALLOC_ANY_TILED;
>> + MIPTREE_LAYOUT_TILING_ANY;
>>
>> intel_miptree_get_dimensions_for_image(rb->TexImage, &width, &height,
>> &depth);
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> index cb2791d..e85c3f0 100644
>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> @@ -455,7 +455,7 @@ intel_miptree_create_layout(struct brw_context *brw,
>> uint32_t stencil_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD;
>> if (brw->gen == 6) {
>> stencil_flags |= MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD |
>> - MIPTREE_LAYOUT_ALLOC_ANY_TILED;
>> + MIPTREE_LAYOUT_TILING_ANY;
>> }
>>
>> mt->stencil_mt = intel_miptree_create(brw,
>> @@ -759,8 +759,8 @@ intel_miptree_create_for_bo(struct brw_context *brw,
>> /* The BO already has a tiling format and we shouldn't confuse the lower
>> * layers by making it try to find a tiling format again.
>> */
>> - assert((layout_flags & MIPTREE_LAYOUT_ALLOC_ANY_TILED) == 0);
>> - assert((layout_flags & MIPTREE_LAYOUT_ALLOC_LINEAR) == 0);
>> + assert((layout_flags & MIPTREE_LAYOUT_TILING_ANY) == 0);
>> + assert((layout_flags & MIPTREE_LAYOUT_TILING_NONE) == 0);
>>
>> layout_flags |= MIPTREE_LAYOUT_FOR_BO;
>> mt = intel_miptree_create_layout(brw, target, format,
>> @@ -874,7 +874,7 @@ intel_miptree_create_for_renderbuffer(struct brw_context
>> *brw,
>> bool ok;
>> GLenum target = num_samples > 1 ? GL_TEXTURE_2D_MULTISAMPLE :
>> GL_TEXTURE_2D;
>> const uint32_t layout_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD |
>> - MIPTREE_LAYOUT_ALLOC_ANY_TILED;
>> + MIPTREE_LAYOUT_TILING_ANY;
>>
>>
>> mt = intel_miptree_create(brw, target, format, 0, 0,
>> @@ -1385,7 +1385,7 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
>> * "The MCS surface must be stored as Tile Y."
>> */
>> const uint32_t mcs_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD |
>> - MIPTREE_LAYOUT_ALLOC_YTILED;
>> + MIPTREE_LAYOUT_TILING_Y;
>> mt->mcs_mt = intel_miptree_create(brw,
>> mt->target,
>> format,
>> @@ -1444,7 +1444,7 @@ intel_miptree_alloc_non_msrt_mcs(struct brw_context
>> *brw,
>> ALIGN(mt->logical_height0, height_divisor) / height_divisor;
>> assert(mt->logical_depth0 == 1);
>> uint32_t layout_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD |
>> - MIPTREE_LAYOUT_ALLOC_YTILED;
>> + MIPTREE_LAYOUT_TILING_Y;
>> if (brw->gen >= 8) {
>> layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
>> }
>> @@ -1709,7 +1709,7 @@ intel_hiz_miptree_buf_create(struct brw_context *brw,
>> if (!buf)
>> return NULL;
>>
>> - layout_flags |= MIPTREE_LAYOUT_ALLOC_ANY_TILED;
>> + layout_flags |= MIPTREE_LAYOUT_TILING_ANY;
>> buf->mt = intel_miptree_create(brw,
>> mt->target,
>> mt->format,
>> @@ -2149,7 +2149,7 @@ intel_miptree_map_blit(struct brw_context *brw,
>> map->mt = intel_miptree_create(brw, GL_TEXTURE_2D, mt->format,
>> 0, 0,
>> map->w, map->h, 1,
>> - 0, MIPTREE_LAYOUT_ALLOC_LINEAR);
>> + 0, MIPTREE_LAYOUT_TILING_NONE);
>>
>> if (!map->mt) {
>> fprintf(stderr, "Failed to allocate blit temporary\n");
>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
>> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
>> index 506c258..790d312 100644
>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
>> @@ -536,10 +536,10 @@ enum {
>> MIPTREE_LAYOUT_DISABLE_AUX = 1 << 3,
>> MIPTREE_LAYOUT_FORCE_HALIGN16 = 1 << 4,
>>
>> - MIPTREE_LAYOUT_ALLOC_YTILED = 1 << 5,
>> - MIPTREE_LAYOUT_ALLOC_LINEAR = 1 << 6,
>> - MIPTREE_LAYOUT_ALLOC_ANY_TILED = MIPTREE_LAYOUT_ALLOC_YTILED |
>> - MIPTREE_LAYOUT_ALLOC_LINEAR,
>> + MIPTREE_LAYOUT_TILING_Y = 1 << 5,
>> + MIPTREE_LAYOUT_TILING_NONE = 1 << 6,
>> + MIPTREE_LAYOUT_TILING_ANY = MIPTREE_LAYOUT_TILING_Y |
>> + MIPTREE_LAYOUT_TILING_NONE,
>
> Can you add a comment about why "ANY" doesn't really mean any? (X-tiling is
> useless garbage)
Well, it kind of does -- passing ANY to brw_miptree_choose_tiling()
can select X tiling. It's just because we have no users of
brw_miptree_choose_tiling() that specifically request X tiling that we
don't have a bit for it or code that handles it.
I can add a comment saying brw_miptree_choose_tiling() doesn't handle
X-tiling and thus there's no enum, but it seems kind of self-evident
by the lack of the enum.
(In fact, it seems that we should have had that comment when we had an
X-tiling enum value that wasn't handled)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
