[Mesa-dev] [PATCH 4/5] i965/miptree: Rename align_w, align_h -> halign, valign
Ben Widawsky
ben at bwidawsk.net
Sun Sep 27 09:26:56 PDT 2015
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.
> + *
> + * 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. By the way, I'm not sure what
motivated you to change the type to uint32_t. It's definitely a matter
of preference, but I am beginning to favor "unsigned" for situations
where we don't actually care about the storage size.
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
More information about the mesa-dev
mailing list