On Thu, Aug 06, 2015 at 11:39:04AM -0700, Matt Turner wrote:
> 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?
>
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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
