[Mesa-dev] [PATCH] i965: Rename MIPTREE_LAYOUT_ALLOC_* -> MIPTREE_LAYOUT_TILING_*.
Ben Widawsky
ben at bwidawsk.net
Thu Aug 6 12:23:17 PDT 2015
On Thu, Aug 06, 2015 at 11:39:04AM -0700, Matt Turner wrote:
> On Thu, Aug 6, 2015 at 11:14 AM, Ben Widawsky <ben at 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?
>
Sorry, I didn't realize this was on top of your previous patch to change the
meaning of "ANY." You're correct, no change here.
> >>
> >> 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)
self-evidence is relative I'd say. If you don't want to add it, I can live with
that.
My thought is just looking at the enum. It says to me "any tiling mode
, which actually expands to *not* any of the tiling modes, and an untiled tiling
mode. Anyway, it's really all bikeshedding at this point, so I don't care too
much - mostly just explaining my original thoughts. I am beginning to see that
our differing views on it might be that I'm thinking of what the hardware
supports, and you're thinking of what the current driver supports.
More information about the mesa-dev
mailing list