On Sat 09 Jul 2016, Jason Ekstrand wrote:
> This is based on a very long set of discussions between Chad and myself
> about how we should properly represent HiZ and CCS buffers. The end result
> of that discussion was that a tiling actually has two different sizes, a
> logical size in elements, and a physical size in bytes and rows. This
> commit reworks ISL's pitch and size calculations to work in terms of these
> two sizes.
> ---
> src/intel/isl/isl.c | 181
> ++++++++++++++++++++++++++++++----------------------
> src/intel/isl/isl.h | 32 +++++++++-
> 2 files changed, 133 insertions(+), 80 deletions(-)
>
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 6f57ac2..633bfdf 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -111,30 +111,32 @@ isl_tiling_get_info(const struct isl_device *dev,
> struct isl_tile_info *tile_info)
> {
> case ISL_TILING_W:
> /* XXX: Should W tile be same as Y? */
> - width = 1 << 6;
> - height = 1 << 6;
> + assert(bs == 1);
> + logical_el = isl_extent2d(64, 64);
> + phys_B = isl_extent2d(64, 64);
> break;
I see you clean up W-tiling patch 5. So this looks ok.
> @@ -1094,10 +1083,6 @@ isl_surf_init_s(const struct isl_device *dev,
> assert(phys_slice0_sa.w % fmtl->bw == 0);
> assert(phys_slice0_sa.h % fmtl->bh == 0);
>
> - const uint32_t row_pitch = isl_calc_row_pitch(dev, info, &tile_info,
> - &image_align_sa,
> - &phys_slice0_sa);
> -
> const uint32_t array_pitch_el_rows =
> isl_calc_array_pitch_el_rows(dev, info, &tile_info, dim_layout,
> array_pitch_span, &image_align_sa,
> @@ -1108,16 +1093,50 @@ isl_surf_init_s(const struct isl_device *dev,
> uint32_t pad_bytes;
> isl_apply_surface_padding(dev, info, &tile_info, &total_h_el, &pad_bytes);
>
> - /* Be sloppy. Align any leftover padding to a row boundary. */
> - total_h_el += isl_align_div_npot(pad_bytes, row_pitch);
> + uint32_t row_pitch, size, base_alignment;
> + if (tiling == ISL_TILING_LINEAR) {
> + row_pitch = isl_calc_linear_row_pitch(dev, info, &phys_slice0_sa);
> + size = row_pitch * total_h_el + pad_bytes;
>
> - const uint32_t size =
> - row_pitch * isl_align(total_h_el, tile_info.height);
> + /* From the Broadwell PRM Vol 2d,
> RENDER_SURFACE_STATE::SurfaceBaseAddress:
> + *
> + * "The Base Address for linear render target surfaces and surfaces
> + * accessed with the typed surface read/write data port messages
> must
> + * be element-size aligned, for non-YUV surface formats, or a
> + * multiple of 2 element-sizes for YUV surface formats. Other linear
> + * surfaces have no alignment requirements (byte alignment is
> + * sufficient.)"
> + */
> + base_alignment = MAX(1, info->min_alignment);
> + if (info->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) {
> + if (isl_format_is_yuv(info->format)) {
> + base_alignment = MAX(base_alignment, 2 * fmtl->bs);
> + } else {
> + base_alignment = MAX(base_alignment, fmtl->bs);
> + }
> + }
> + } else {
> + assert(phys_slice0_sa.w % fmtl->bw == 0);
> + const uint32_t total_w_el = phys_slice0_sa.width / fmtl->bw;
> + const uint32_t total_w_tl =
> + isl_align_div(total_w_el, tile_info.logical_extent_el.width);
> +
> + row_pitch = total_w_tl * tile_info.phys_extent_B.width;
> + if (row_pitch < info->min_pitch) {
> + row_pitch = isl_align(info->min_pitch,
> tile_info.phys_extent_B.width);
> + }
>
> - /* Alignment of surface base address, in bytes */
> - uint32_t base_alignment = MAX(1, info->min_alignment);
> - assert(isl_is_pow2(base_alignment) && isl_is_pow2(tile_info.size));
> - base_alignment = MAX(base_alignment, tile_info.size);
> + total_h_el += isl_align_div_npot(pad_bytes, row_pitch);
> + const uint32_t total_h_tl =
> + isl_align_div(total_h_el, tile_info.logical_extent_el.height);
> +
> + size = total_h_tl * tile_info.phys_extent_B.height * row_pitch;
> +
> + const uint32_t tile_size = tile_info.phys_extent_B.width *
> + tile_info.phys_extent_B.height;
> + assert(isl_is_pow2(info->min_alignment) && isl_is_pow2(tile_size));
> + base_alignment = MAX(info->min_alignment, tile_size);
> + }
The calculations look correct to me. The unit suffixes really help the
verification.
> @@ -1420,9 +1439,6 @@ isl_tiling_get_intratile_offset_el(const struct
> isl_device *dev,
> uint32_t *x_offset_el,
> uint32_t *y_offset_el)
> {
> - struct isl_tile_info tile_info;
> - isl_tiling_get_info(dev, tiling, bs, &tile_info);
> -
> /* This function only really works for power-of-two surfaces. In
> * theory, we could make it work for non-power-of-two surfaces by going
> * to the left until we find a block that is bs-aligned. The Vulkan
> @@ -1431,18 +1447,29 @@ isl_tiling_get_intratile_offset_el(const struct
> isl_device *dev,
> */
> assert(tiling == ISL_TILING_LINEAR || isl_is_pow2(bs));
>
> - uint32_t small_y_offset_el = total_y_offset_el % tile_info.height;
> - uint32_t big_y_offset_el = total_y_offset_el - small_y_offset_el;
> - uint32_t big_y_offset_B = big_y_offset_el * row_pitch;
> + if (tiling == ISL_TILING_LINEAR) {
> + *base_address_offset = total_y_offset_el * row_pitch +
> + total_x_offset_el * bs;
> + *x_offset_el = 0;
> + *y_offset_el = 0;
> + return;
> + }
> +
> + struct isl_tile_info tile_info;
> + isl_tiling_get_info(dev, tiling, bs, &tile_info);
> +
> + /* Compute the offset into the tile */
> + *x_offset_el = total_x_offset_el % tile_info.logical_extent_el.w;
> + *y_offset_el = total_y_offset_el % tile_info.logical_extent_el.h;
>
> - uint32_t total_x_offset_B = total_x_offset_el * bs;
> - uint32_t small_x_offset_B = total_x_offset_B % tile_info.width;
> - uint32_t small_x_offset_el = small_x_offset_B / bs;
> - uint32_t big_x_offset_B = (total_x_offset_B / tile_info.width) *
> tile_info.size;
> + /* Compute the offset of the tile in units of whole tiles */
> + uint32_t x_offset_tl = total_x_offset_el / tile_info.logical_extent_el.w;
> + uint32_t y_offset_tl = total_y_offset_el / tile_info.logical_extent_el.h;
>
> - *base_address_offset = big_y_offset_B + big_x_offset_B;
> - *x_offset_el = small_x_offset_el;
> - *y_offset_el = small_y_offset_el;
> + assert(row_pitch % tile_info.phys_extent_B.width == 0);
> + *base_address_offset =
> + y_offset_tl * tile_info.phys_extent_B.h * row_pitch +
> + x_offset_tl * tile_info.phys_extent_B.h * tile_info.phys_extent_B.w;
> }
I was initially afraid that the new isl_tile_info concepts would lead to
confusing code. To the contrary, this function proves the new concept
leads to very readable code that clearly expresses the calculations.
> uint32_t
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> index 985b605..6ec956b 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -660,9 +660,28 @@ struct isl_format_layout {
>
> struct isl_tile_info {
> enum isl_tiling tiling;
> - uint32_t width; /**< in bytes */
> - uint32_t height; /**< in rows of memory */
> - uint32_t size; /**< in bytes */
> +
> + /** The logical size of the tile in units of surface elements
> + *
> + * This field determines how a given surface is cut up into tiles. It is
> + * used to compute the size of a surface in tiles and can be used to
> + * determine the location of the tile containing any given surface
> element.
> + * The exact value of this field depends heavily on the bits-per-block of
> + * the format being used.
> + */
> + struct isl_extent2d logical_extent_el;
> +
> + /** The physical size of the tile in bytes and rows of bytes
> + *
> + * This field determines how the tiles of a surface are physically layed
> + * out in memory. The logical and physical tile extent are frequently the
> + * same but this is not always the case. For instance, a W-tile (which is
> + * always used with ISL_FORMAT_R8) has a logical size of 64el x 64el but
> + * its physical size is 128B x 32rows, the same as a Y-tile.
> + *
> + * @see isl_surf::row_pitch
> + */
> + struct isl_extent2d phys_extent_B;
These comments look good.
> };
>
> /**
> @@ -744,6 +763,13 @@ struct isl_surf {
>
> /**
> * Pitch between vertically adjacent surface elements, in bytes.
> + *
> + * For tiled surfaces, the interpretation of this value depends on the
> + * value of isl_tile_info::physical_extent_B. In particular, the distance
> + * in bytes between vertically adjacent tiles in the image is given by
> + * row_pitch * isl_tile_info::physical_extent_B.height.
> + *
> + * @see isl_tile_info::phys_extent_B;
> */
> uint32_t row_pitch;
Everything in the patch looked good, until I reached this final hunk.
The comment confuses me; it seems to contradict itself. The comment
first says the unit of row_pitch is B / el_rows; later it says the unit
is B / physical_B_rows. (I'm inventing units here). My review here is
not merely pedantic; I honestly no longer know what isl_surf::row_pitch
is anymore.
I searched for a clue by checking out your branch and grepping for
row_pitch, but that led to more confusion. I found this line, which
makes sense to me:
row_pitch = total_w_tl * tile_info.phy_extent_B.width
In that line, the row_pitch unit is (loosely) B / physical_b_rows.
But I also found this line:
size = row_pitch * total_h_el + pad_bytes
In that line, the row_pitchunit is B / el_rows
HELP!?!?!?
Could you clarify the comment and perhaps provide an example?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
