[Mesa-dev] [PATCH 4/5] i965/miptree: Rename align_w, align_h -> halign, valign
Chad Versace
chad.versace at intel.com
Tue Sep 29 14:02:48 PDT 2015
On Sun 27 Sep 2015, Ben Widawsky wrote:
> On Fri, Sep 25, 2015 at 12:05:49PM -0700, Chad Versace wrote:
> > The values of intel_mipmap_tree::align_w and ::align_h correspond to the
> > hardware enums HALIGN_* and VALIGN_*.
> >
> > See the confusion?
> > align_h != HALIGN
> > align_h == VALIGN
> >
> > Reduce the confusion by renaming the variables to match the hardware
> > enum names:
> > git ls-files |
> > xargs sed -i -e 's/align_w/halign/g' \
> > -e 's/align_h/valign/g'
> >
> > Suggested-by: Kenneth Graunke <kenneth at whitecape.org>
> > Cc: Ben Widawsky <ben at bwidawsk.net>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> > src/mesa/drivers/dri/i965/brw_tex_layout.c | 62 +++++++++++------------
> > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 4 +-
> > src/mesa/drivers/dri/i965/gen6_blorp.cpp | 2 +-
> > src/mesa/drivers/dri/i965/gen6_surface_state.c | 2 +-
> > src/mesa/drivers/dri/i965/gen7_blorp.cpp | 4 +-
> > src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 8 +--
> > src/mesa/drivers/dri/i965/gen8_surface_state.c | 12 ++---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 2 +-
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 18 +++++--
> > 9 files changed, 62 insertions(+), 52 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > index 268b995..2955c8d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > @@ -282,7 +282,7 @@ gen9_miptree_layout_1d(struct intel_mipmap_tree *mt)
> > /* When this layout is used the horizontal alignment is fixed at 64 and the
> > * hardware ignores the value given in the surface state
> > */
> > - const unsigned int align_w = 64;
> > + const unsigned int halign = 64;
> >
> > mt->total_height = mt->physical_height0;
> > mt->total_width = 0;
> > @@ -292,7 +292,7 @@ gen9_miptree_layout_1d(struct intel_mipmap_tree *mt)
> >
> > intel_miptree_set_level_info(mt, level, x, 0, depth);
> >
> > - img_width = ALIGN(width, align_w);
> > + img_width = ALIGN(width, halign);
> >
> > mt->total_width = MAX2(mt->total_width, x + img_width);
> >
> > @@ -328,10 +328,10 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
> > unsigned mip1_width;
> >
> > if (mt->compressed) {
> > - mip1_width = ALIGN_NPOT(minify(mt->physical_width0, 1), mt->align_w) +
> > + mip1_width = ALIGN_NPOT(minify(mt->physical_width0, 1), mt->halign) +
> > ALIGN_NPOT(minify(mt->physical_width0, 2), bw);
> > } else {
> > - mip1_width = ALIGN_NPOT(minify(mt->physical_width0, 1), mt->align_w) +
> > + mip1_width = ALIGN_NPOT(minify(mt->physical_width0, 1), mt->halign) +
> > minify(mt->physical_width0, 2);
> > }
> >
> > @@ -348,7 +348,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
> >
> > intel_miptree_set_level_info(mt, level, x, y, depth);
> >
> > - img_height = ALIGN_NPOT(height, mt->align_h);
> > + img_height = ALIGN_NPOT(height, mt->valign);
> > if (mt->compressed)
> > img_height /= bh;
> >
> > @@ -365,7 +365,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
> > /* Layout_below: step right after second mipmap.
> > */
> > if (level == mt->first_level + 1) {
> > - x += ALIGN_NPOT(width, mt->align_w) / bw;
> > + x += ALIGN_NPOT(width, mt->halign) / bw;
> > } else {
> > y += img_height;
> > }
> > @@ -385,7 +385,7 @@ brw_miptree_get_horizontal_slice_pitch(const struct brw_context *brw,
> > {
> > if ((brw->gen < 9 && mt->target == GL_TEXTURE_3D) ||
> > (brw->gen == 4 && mt->target == GL_TEXTURE_CUBE_MAP)) {
> > - return ALIGN_NPOT(minify(mt->physical_width0, level), mt->align_w);
> > + return ALIGN_NPOT(minify(mt->physical_width0, level), mt->halign);
> > } else {
> > return 0;
> > }
> > @@ -426,13 +426,13 @@ brw_miptree_get_vertical_slice_pitch(const struct brw_context *brw,
> > } else if (mt->target == GL_TEXTURE_3D ||
> > (brw->gen == 4 && mt->target == GL_TEXTURE_CUBE_MAP) ||
> > mt->array_layout == ALL_SLICES_AT_EACH_LOD) {
> > - return ALIGN_NPOT(minify(mt->physical_height0, level), mt->align_h);
> > + return ALIGN_NPOT(minify(mt->physical_height0, level), mt->valign);
> >
> > } else {
> > - const unsigned h0 = ALIGN_NPOT(mt->physical_height0, mt->align_h);
> > - const unsigned h1 = ALIGN_NPOT(minify(mt->physical_height0, 1), mt->align_h);
> > + const unsigned h0 = ALIGN_NPOT(mt->physical_height0, mt->valign);
> > + const unsigned h1 = ALIGN_NPOT(minify(mt->physical_height0, 1), mt->valign);
> >
> > - return h0 + h1 + (brw->gen >= 7 ? 12 : 11) * mt->align_h;
> > + return h0 + h1 + (brw->gen >= 7 ? 12 : 11) * mt->valign;
> > }
> > }
> >
> > @@ -502,9 +502,9 @@ brw_miptree_layout_texture_array(struct brw_context *brw,
> >
> > for (unsigned level = mt->first_level; level <= mt->last_level; level++) {
> > unsigned img_height;
> > - img_height = ALIGN_NPOT(height, mt->align_h);
> > + img_height = ALIGN_NPOT(height, mt->valign);
> > if (mt->compressed)
> > - img_height /= mt->align_h;
> > + img_height /= mt->valign;
> >
> > for (unsigned q = 0; q < mt->level[level].depth; q++) {
> > if (mt->array_layout == ALL_SLICES_AT_EACH_LOD) {
> > @@ -537,8 +537,8 @@ brw_miptree_layout_texture_3d(struct brw_context *brw,
> > unsigned WL = MAX2(mt->physical_width0 >> level, 1);
> > unsigned HL = MAX2(mt->physical_height0 >> level, 1);
> > unsigned DL = MAX2(mt->physical_depth0 >> level, 1);
> > - unsigned wL = ALIGN_NPOT(WL, mt->align_w);
> > - unsigned hL = ALIGN_NPOT(HL, mt->align_h);
> > + unsigned wL = ALIGN_NPOT(WL, mt->halign);
> > + unsigned hL = ALIGN_NPOT(HL, mt->valign);
> >
> > if (mt->target == GL_TEXTURE_CUBE_MAP)
> > DL = 6;
> > @@ -656,7 +656,7 @@ brw_miptree_choose_tiling(struct brw_context *brw,
> > * to know that ahead of time. And besides, since we use a vertical
> > * alignment of 4 as often as we can, this shouldn't happen very often.
> > */
> > - if (brw->gen == 7 && mt->align_h == 2 &&
> > + if (brw->gen == 7 && mt->valign == 2 &&
> > brw->format_supported_as_render_target[mt->format]) {
> > return I915_TILING_X;
> > }
> > @@ -748,21 +748,21 @@ intel_miptree_set_alignment(struct brw_context *brw,
> > /* Stencil uses W tiling, so we force W tiling alignment for the
> > * ALL_SLICES_AT_EACH_LOD miptree layout.
> > */
> > - mt->align_w = 64;
> > - mt->align_h = 64;
> > + mt->halign = 64;
> > + mt->valign = 64;
> > assert((layout_flags & MIPTREE_LAYOUT_FORCE_HALIGN16) == 0);
> > } else {
> > /* Depth uses Y tiling, so we force need Y tiling alignment for the
> > * ALL_SLICES_AT_EACH_LOD miptree layout.
> > */
> > - mt->align_w = 128 / mt->cpp;
> > - mt->align_h = 32;
> > + mt->halign = 128 / mt->cpp;
> > + mt->valign = 32;
> > }
> > } else if (mt->compressed) {
> > /* The hardware alignment requirements for compressed textures
> > * happen to match the block boundaries.
> > */
> > - _mesa_get_format_block_size(mt->format, &mt->align_w, &mt->align_h);
> > + _mesa_get_format_block_size(mt->format, &mt->halign, &mt->valign);
> >
> > /* On Gen9+ we can pick our own alignment for compressed textures but it
> > * has to be a multiple of the block size. The minimum alignment we can
> > @@ -770,21 +770,21 @@ intel_miptree_set_alignment(struct brw_context *brw,
> > * size
> > */
> > if (brw->gen >= 9) {
> > - mt->align_w *= 4;
> > - mt->align_h *= 4;
> > + mt->halign *= 4;
> > + mt->valign *= 4;
> > }
> > } else if (mt->format == MESA_FORMAT_S_UINT8) {
> > - mt->align_w = 8;
> > - mt->align_h = brw->gen >= 7 ? 8 : 4;
> > + mt->halign = 8;
> > + mt->valign = brw->gen >= 7 ? 8 : 4;
> > } else if (brw->gen >= 9 && mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE) {
> > /* XY_FAST_COPY_BLT doesn't support horizontal alignment < 32 or
> > * vertical alignment < 64. */
> > - mt->align_w = MAX2(tr_mode_horizontal_texture_alignment(brw, mt), 32);
> > - mt->align_h = MAX2(tr_mode_vertical_texture_alignment(brw, mt), 64);
> > + mt->halign = MAX2(tr_mode_horizontal_texture_alignment(brw, mt), 32);
> > + mt->valign = MAX2(tr_mode_vertical_texture_alignment(brw, mt), 64);
> > } else {
> > - mt->align_w =
> > + mt->halign =
> > intel_horizontal_texture_alignment_unit(brw, mt, layout_flags);
> > - mt->align_h = intel_vertical_texture_alignment_unit(brw, mt);
> > + mt->valign = intel_vertical_texture_alignment_unit(brw, mt);
> > }
> > }
> >
> > @@ -809,8 +809,8 @@ brw_miptree_layout(struct brw_context *brw,
> > if (brw->gen >= 9) {
> > unsigned int i, j;
> > _mesa_get_format_block_size(mt->format, &i, &j);
> > - mt->align_w /= i;
> > - mt->align_h /= j;
> > + mt->halign /= i;
> > + mt->valign /= j;
> > }
> >
> > if ((layout_flags & MIPTREE_LAYOUT_FOR_BO) == 0)
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > index 8213f4e..c11925c 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -380,7 +380,7 @@ brw_update_texture_surface(struct gl_context *ctx,
> > surf[4] = (brw_get_surface_num_multisamples(mt->num_samples) |
> > SET_FIELD(tObj->BaseLevel - mt->first_level, BRW_SURFACE_MIN_LOD));
> >
> > - surf[5] = mt->align_h == 4 ? BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0;
> > + surf[5] = mt->valign == 4 ? BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0;
> >
> > /* Emit relocation to surface contents */
> > drm_intel_bo_emit_reloc(brw->batch.bo,
> > @@ -695,7 +695,7 @@ brw_update_renderbuffer_surface(struct brw_context *brw,
> > assert(tile_y % 2 == 0);
> > surf[5] = ((tile_x / 4) << BRW_SURFACE_X_OFFSET_SHIFT |
> > (tile_y / 2) << BRW_SURFACE_Y_OFFSET_SHIFT |
> > - (mt->align_h == 4 ? BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0));
> > + (mt->valign == 4 ? BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0));
> >
> > if (brw->gen < 6) {
> > /* _NEW_COLOR */
> > diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> > index cba5c2f..74f3a70 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> > +++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> > @@ -413,7 +413,7 @@ gen6_blorp_emit_surface_state(struct brw_context *brw,
> > assert(tile_y % 2 == 0);
> > surf[5] = ((tile_x / 4) << BRW_SURFACE_X_OFFSET_SHIFT |
> > (tile_y / 2) << BRW_SURFACE_Y_OFFSET_SHIFT |
> > - (surface->mt->align_h == 4 ?
> > + (surface->mt->valign == 4 ?
> > BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0));
> >
> > /* Emit relocation to surface contents */
> > diff --git a/src/mesa/drivers/dri/i965/gen6_surface_state.c b/src/mesa/drivers/dri/i965/gen6_surface_state.c
> > index 39de62f..d892c93 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_surface_state.c
> > @@ -126,7 +126,7 @@ gen6_update_renderbuffer_surface(struct brw_context *brw,
> > SET_FIELD(min_array_element, BRW_SURFACE_MIN_ARRAY_ELEMENT) |
> > SET_FIELD(depth - 1, BRW_SURFACE_RENDER_TARGET_VIEW_EXTENT);
> >
> > - surf[5] = (mt->align_h == 4 ? BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0);
> > + surf[5] = (mt->valign == 4 ? BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0);
> >
> > drm_intel_bo_emit_reloc(brw->batch.bo,
> > offset + 4,
> > diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > index 9822dc1..f90e78e 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> > @@ -158,9 +158,9 @@ gen7_blorp_emit_surface_state(struct brw_context *brw,
> > surface->brw_surfaceformat << BRW_SURFACE_FORMAT_SHIFT |
> > gen7_surface_tiling_mode(tiling);
> >
> > - if (surface->mt->align_h == 4)
> > + if (surface->mt->valign == 4)
> > surf[0] |= GEN7_SURFACE_VALIGN_4;
> > - if (surface->mt->align_w == 8)
> > + if (surface->mt->halign == 8)
> > surf[0] |= GEN7_SURFACE_HALIGN_8;
> >
> > if (surface->array_layout == ALL_SLICES_AT_EACH_LOD)
> > diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> > index 15ab2b0..5080f1c 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> > @@ -288,9 +288,9 @@ gen7_emit_texture_surface_state(struct brw_context *brw,
> > if (target == GL_TEXTURE_CUBE_MAP || target == GL_TEXTURE_CUBE_MAP_ARRAY)
> > surf[0] |= BRW_SURFACE_CUBEFACE_ENABLES;
> >
> > - if (mt->align_h == 4)
> > + if (mt->valign == 4)
> > surf[0] |= GEN7_SURFACE_VALIGN_4;
> > - if (mt->align_w == 8)
> > + if (mt->halign == 8)
> > surf[0] |= GEN7_SURFACE_HALIGN_8;
> >
> > if (_mesa_is_array_texture(target) || target == GL_TEXTURE_CUBE_MAP)
> > @@ -509,9 +509,9 @@ gen7_update_renderbuffer_surface(struct brw_context *brw,
> > GEN7_SURFACE_ARYSPC_LOD0 : GEN7_SURFACE_ARYSPC_FULL) |
> > gen7_surface_tiling_mode(mt->tiling);
> >
> > - if (irb->mt->align_h == 4)
> > + if (irb->mt->valign == 4)
> > surf[0] |= GEN7_SURFACE_VALIGN_4;
> > - if (irb->mt->align_w == 8)
> > + if (irb->mt->halign == 8)
> > surf[0] |= GEN7_SURFACE_HALIGN_8;
> >
> > if (is_array) {
> > diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > index d2f333f..1a4b517 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > @@ -95,7 +95,7 @@ vertical_alignment(const struct brw_context *brw,
> > surf_type == BRW_SURFACE_1D))
> > return GEN8_SURFACE_VALIGN_4;
> >
> > - switch (mt->align_h) {
> > + switch (mt->valign) {
> > case 4:
> > return GEN8_SURFACE_VALIGN_4;
> > case 8:
> > @@ -120,7 +120,7 @@ horizontal_alignment(const struct brw_context *brw,
> > gen9_use_linear_1d_layout(brw, mt)))
> > return GEN8_SURFACE_HALIGN_4;
> >
> > - switch (mt->align_w) {
> > + switch (mt->halign) {
> > case 4:
> > return GEN8_SURFACE_HALIGN_4;
> > case 8:
> > @@ -221,8 +221,8 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
> > * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
> > * 16 must be used."
> > */
> > - assert(brw->gen < 9 || mt->align_w == 16);
> > - assert(brw->gen < 8 || mt->num_samples > 1 || mt->align_w == 16);
> > + assert(brw->gen < 9 || mt->halign == 16);
> > + assert(brw->gen < 8 || mt->num_samples > 1 || mt->halign == 16);
> > }
> >
> > const uint32_t surf_type = translate_tex_target(target);
> > @@ -465,8 +465,8 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
> > * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
> > * 16 must be used."
> > */
> > - assert(brw->gen < 9 || mt->align_w == 16);
> > - assert(brw->gen < 8 || mt->num_samples > 1 || mt->align_w == 16);
> > + assert(brw->gen < 9 || mt->halign == 16);
> > + assert(brw->gen < 8 || mt->num_samples > 1 || mt->halign == 16);
> > }
> >
> > uint32_t *surf = allocate_surface_state(brw, &offset, surf_index);
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 29e5300..d29bf01 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -712,7 +712,7 @@ intel_miptree_create(struct brw_context *brw,
> > if (intel_tiling_supports_non_msrt_mcs(brw, mt->tiling) &&
> > intel_miptree_is_fast_clear_capable(brw, mt)) {
> > mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_RESOLVED;
> > - assert(brw->gen < 8 || mt->align_w == 16 || num_samples <= 1);
> > + assert(brw->gen < 8 || mt->halign == 16 || num_samples <= 1);
> > }
> >
> > return mt;
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > index 9f5397f..bfcecea 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > @@ -373,11 +373,21 @@ struct intel_mipmap_tree
> > mesa_format etc_format;
> >
> > /**
> > - * The X offset of each image in the miptree must be aligned to this.
> > - * See the comments in brw_tex_layout.c.
> > + * @name Surface Alignment
> > + * @{
> > + *
> > + * This defines the alignment of the upperleft pixel of each 2D subimage
> > + * contained in the surface. The alignment is in pixel coordinates relative
> > + * to the surface's most upperleft pixel, which is the pixel at (x=0, y=0,
> > + * layer=0, level=0).
>
> I'd just kill the first sentence above. I think instead of "2D subimage"
> you should just say "surface" because I find this too confusing for 1d
> and 3d surfaces, even though I do see the distinction you're trying to
> make. The second sentence is clear, and sufficient.
Using the term "surface" here would be incorrect, as the "surface"
properly refers to the entire thing, including every array slice and
z plane, and does not refer to a 2D thing. The hw docs consistently use
the term "surface" in that way, and this patch series' goal is all about
making code and comments align better with the hw docs. (For example,
the hw docs contain phrases like "3D surface" and "array surface").
The first sentence is important, because without it, there's no
indication of *what* the alignment applies to. Does it apply to the
upperleft pixel of the surface? No. To the upperleft pixel of each array
slice? No. To the upperleft pixel of each miplevel? No.
The alignment applies to the upperleft pixel of each, um, that thing
we're having trouble naming.
Since the aligned "thing" can't be called "surface", and it can't be
called "2D subimage" (I admit that's a confusing term), do you have any
other ideas? The hw docs for 3D surfaces use the term "plane", but that
may also be confusing.
>
> > + *
> > + * In the surface layout equations found in the hardware docs, the
> > + * horizontal and vertical surface alignments often appear as variables 'i'
> > + * and 'j'.
> > */
> > - unsigned int align_w;
> > - unsigned int align_h; /**< \see align_w */
> > + uint32_t halign; /**< RENDER_SURFACE_STATE.SurfaceHorizontalAlignment */
> > + uint32_t valign; /**< RENDER_SURFACE_STATE.SurfaceVerticalAlignment */
> > + /** @} */
> >
> > GLuint first_level;
> > GLuint last_level;
>
> In fairness to whomever originally wrote the code, after you make the
> mistake once, it really isn't that bad.
It was me, in 2011 :) See commit 2361a497d2c2d9de938a8a5af8a9fcd0e35348c9.
And it still confuses me occasionally.
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
More information about the mesa-dev
mailing list