[Mesa-dev] [PATCH 2/5] i965/gen9: Plugin the code for selecting YF/YS tiling on skl+
Kenneth Graunke
kenneth at whitecape.org
Tue Jul 7 02:47:40 PDT 2015
On Wednesday, June 10, 2015 03:30:47 PM Anuj Phogat wrote:
> Buffers with Yf/Ys tiling end up using meta upload / download
> paths or the blitter for cases where they used tiled_memcpy paths
> in case of Y tiling. This has exposed some bugs in meta path. To
> avoid any piglit regressions on SKL this patch keeps the Yf/Ys
> tiling disabled at the moment.
>
> V3: Make brw_miptree_choose_tr_mode() actually choose TRMODE. (Ben)
> Few cosmetic changes.
> V4: Get rid of brw_miptree_choose_tr_mode().
> Take care of all tile resource modes {Yf, Ys, none} for all
> generations at one place.
>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> ---
> src/mesa/drivers/dri/i965/brw_tex_layout.c | 97 ++++++++++++++++++++++++------
> 1 file changed, 79 insertions(+), 18 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index b9ac4cf..c0ef5cc 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -807,27 +807,88 @@ brw_miptree_layout(struct brw_context *brw,
> enum intel_miptree_tiling_mode requested,
> struct intel_mipmap_tree *mt)
> {
> - mt->tr_mode = INTEL_MIPTREE_TRMODE_NONE;
> + const unsigned bpp = mt->cpp * 8;
> + const bool is_tr_mode_yf_ys_allowed =
> + brw->gen >= 9 &&
> + !for_bo &&
> + !mt->compressed &&
> + /* Enable YF/YS tiling only for color surfaces because depth and
> + * stencil surfaces are not supported in blitter using fast copy
> + * blit and meta PBO upload, download paths. No other paths
> + * currently support Yf/Ys tiled surfaces.
> + * FIXME: Remove this restriction once we have a tiled_memcpy()
> + * path to do depth/stencil data upload/download to Yf/Ys tiled
> + * surfaces.
> + */
> + _mesa_is_format_color_format(mt->format) &&
> + (requested == INTEL_MIPTREE_TILING_Y ||
> + requested == INTEL_MIPTREE_TILING_ANY) &&
> + (bpp && is_power_of_two(bpp)) &&
> + /* FIXME: To avoid piglit regressions keep the Yf/Ys tiling
> + * disabled at the moment.
> + */
> + false;
I must say, I was a bit surprised to see this land as is. You've got a
lot of conditions there, only to finish them up with && false - with a
comment saying that your code isn't passing Piglit yet. That doesn't
really meet our usual qualifications for merging.
Coverity also pointed out that your if (is_tr_mode_yf_ys_allowed) block
below is dead code, issuing new warnings.
Forgive my ignorance, but what's the purpose of Yf/Ys tiling? My
understanding was that Ys is primarily in support of a new OpenGL
feature - GL_ARB_spare_texture(*) - which isn't yet enabled:
https://www.opengl.org/registry/specs/ARB/sparse_texture.txt
Is Yf tiling supposed to be more efficient than legacy Y-tiling? If so,
then switching to it is an optimization, isn't it? We usually require
data indicating some kind of performance improvement (any kind!) before
landing a bunch of code for optimizations. Obviously that's pretty
tricky with pre-release hardware, so I'd settle for "it's complete
and functions correctly."
At any rate, it's merged, and hopefully you're able to get it working...
>
> - intel_miptree_set_alignment(brw, mt);
> - intel_miptree_set_total_width_height(brw, mt);
> + /* Lower index (Yf) is the higher priority mode */
> + const uint32_t tr_mode[3] = {INTEL_MIPTREE_TRMODE_YF,
> + INTEL_MIPTREE_TRMODE_YS,
> + INTEL_MIPTREE_TRMODE_NONE};
> + int i = is_tr_mode_yf_ys_allowed ? 0 : ARRAY_SIZE(tr_mode) - 1;
>
> - if (!mt->total_width || !mt->total_height) {
> - intel_miptree_release(&mt);
> - return;
> - }
> + while (i < ARRAY_SIZE(tr_mode)) {
> + if (brw->gen < 9)
> + assert(tr_mode[i] == INTEL_MIPTREE_TRMODE_NONE);
> + else
> + assert(tr_mode[i] == INTEL_MIPTREE_TRMODE_YF ||
> + tr_mode[i] == INTEL_MIPTREE_TRMODE_YS ||
> + tr_mode[i] == INTEL_MIPTREE_TRMODE_NONE);
>
> - /* On Gen9+ the alignment values are expressed in multiples of the block
> - * size
> - */
> - 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->tr_mode = tr_mode[i];
> + intel_miptree_set_alignment(brw, mt);
> + intel_miptree_set_total_width_height(brw, mt);
>
> - if (!for_bo)
> - mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);
> + if (!mt->total_width || !mt->total_height) {
> + intel_miptree_release(&mt);
> + return;
> + }
> +
> + /* On Gen9+ the alignment values are expressed in multiples of the
> + * block size.
> + */
> + 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;
> + }
> +
> + if (!for_bo)
> + mt->tiling = brw_miptree_choose_tiling(brw, requested, mt);
> +
> + if (is_tr_mode_yf_ys_allowed) {
> + unsigned int level = 0;
> + assert(brw->gen >= 9);
> +
> + if (mt->tiling == I915_TILING_Y ||
> + mt->tiling == (I915_TILING_Y | I915_TILING_X) ||
> + mt->tr_mode == INTEL_MIPTREE_TRMODE_NONE) {
> + /* FIXME: Don't allow YS tiling at the moment. Using 64KB tiling
> + * for small textures might result in to memory wastage. Revisit
> + * this condition when we have more information about the specific
> + * cases where using YS over YF will be useful.
> + */
> + if (mt->tr_mode != INTEL_MIPTREE_TRMODE_YS)
> + return;
> + }
> + /* Failed to use selected tr_mode. Free up the memory allocated
> + * for miptree levels in intel_miptree_total_width_height().
> + */
> + for (level = mt->first_level; level <= mt->last_level; level++) {
> + free(mt->level[level].slice);
> + mt->level[level].slice = NULL;
> + }
> + }
> + i++;
> + }
> }
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150707/5a743e4d/attachment.sig>
More information about the mesa-dev
mailing list