On Wed, Feb 13, 2019 at 12:04:59PM +0200, Eleni Maria Stea wrote:
> GPUs Gen < 8 cannot sample ETC2 formats. So far, they converted the
> compressed EAC/ETC2 images to non-compressed RGBA images. When
> GetCompressed* functions were called, the pixels were returned in this
> RGBA format and not the compressed format that was expected.
>
> Trying to fix this problem, we use a secondary shadow miptree to store the
> decompressed data for the rendering and the main miptree to store the
> compressed for the Get functions to work. Each time that the main miptree
> is written with compressed data, we decompress them to RGB and update the
> shadow. Then we use the shadow for rendering.
>
> v2:
> - Fixes in the commit message (Nanley Chery)
> - Reversed the changes in brw_get_texture_swizzle and swapped the b, g
> values at the time that we decompress the data in the function:
> intel_miptree_update_etc_shadow of intel_mipmap_tree.c (Nanley Chery)
> - Simplified the format checks in the miptree_create function of the
> intel_mipmap_tree.c and reserved the call of the
> intel_lower_compressed_format for the case that we are faking the ETC
> support (Nanley Chery)
> - Removed the check for the auxiliary usage for the shadow miptree at
> creation (miptree_create of intel_mipmap_tree.c) as we won't use
> auxiliary buffers with these types of trees (Nanley Chery)
> - Set the etc_format of the non-ETC miptrees to MESA_FORMAT_NONE and
> removed the unecessary checks (Nanley Chery)
> - Fixed an unrelated indentation change (Nanley Chery)
> - Modified the function intel_miptree_finish_write to set the
> mt->shadow_needs_update to true to catch all the cases when we need to
> update the miptree (Nanley Chery)
> - In order to update the shadow miptree during the unmap of the
> main and always map the main (Nanley Chery) the following change was
> necessary: Splitted the previous update function that was updating all
> the mipmap levels and use two functions instead: one that updates one
> level and one that updates all of them. Used the first during unmap
> and the second before the rendering.
> - Removed the BRW_MAP_ETC_BIT flag and the mechanism to decide which
> miptree should be mapped each time and reversed all the changes in the
> higher level texture functions that upload data to textures as they
> aren't needed anymore.
> - Replaced the boolean needs_fake_etc with an inline function that
> checks when we need to fake the ETC compression (Nanley Chery)
> - Removed the initialization of the strides in the update function as
> the values will be overwritten by the intel_miptree_map call (Nanley
> Chery)
> - Used minify instead of division in the new update function
> intel_miptree_update_etc_shadow_levels in intel_mipmap_tree.c (Nanley
> Chery)
> - Removed the depth from the calculation of the number of slices in
> the new update function (intel_miptree_update_etc_shadow_levels of
> intel_mipmap_tree.c) as we don't need to support 3D ETC images.
> (Nanley Chery)
>
> v3:
> - Renamed the rgba_fmt in function miptree_create
> (intel_mipmap_tree.c) to decomp_format as the format is not always in
> rgba order. (Nanley Chery)
> - Documented the new usage for the shadow miptree in the comment above
> the field in the intel_miptree struct in intel_mipmap_tree.h (Nanley
> Chery)
> - Removed the redundant flags from the mapping of the miptrees in
> intel_miptree_update_etc_shadow of intel_mipmap_tree.c (Nanley Chery)
> - Fixed the switch from surface's logical level to physical level in
> the intel_miptree_update_etc_shadow_levels of intel_mipmap_tree.c
> (Nanley Chery)
> - Excluded the Baytrail GPUs from the check for the ETC emulation as
> they support the ETC formats natively. (Nanley Chery)
> - Simplified the check if the format is BGRA in
> intel_miptree_update_etc_shadow of intel_mipmap_tree.c (Nanley Chery)
>
> v4:
> - Removed the functions intel_miptree_(map|unmap)_etc and the check if
> we need to call them as with the new changes, they became unreachable.
> (Nanley Chery)
> - We'd rather calculate the level width and height using the shadow
> miptree instead of the main in intel_miptree_update_etc_shadow_levels of
> intel_mipmap_tree.c (Nanley Chery)
>
> v5:
> - Fixed the levels calculations in intel_mipmap_tree.c (Nanley Chery)
> - Update the flag shadow_needs_update outside the function
> intel_miptree_update_etc_shadow (Nanley Chery)
> - Fixed indentation error (Nanley Cherry)
^
Extra r here.
There's really only one issue here (a call to mt_surf_usage), my other
comments are probably nitpicking so feel free to take them or leave
them.
> ---
> .../drivers/dri/i965/brw_wm_surface_state.c | 5 +-
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 176 +++++++++++-------
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 24 +++
> 3 files changed, 138 insertions(+), 67 deletions(-)
>
> 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 c55182d7ffb..c3d267721e1 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -521,7 +521,7 @@ static void brw_update_texture_surface(struct gl_context
> *ctx,
> */
> mesa_fmt = mt->format;
> } else if (mt->etc_format != MESA_FORMAT_NONE) {
> - mesa_fmt = mt->format;
> + mesa_fmt = mt->shadow_mt->format;
> } else if (plane > 0) {
> mesa_fmt = mt->format;
> } else {
> @@ -581,6 +581,9 @@ static void brw_update_texture_surface(struct gl_context
> *ctx,
> assert(mt->shadow_mt && !mt->shadow_needs_update);
> mt = mt->shadow_mt;
> format = ISL_FORMAT_R8_UINT;
> + } else if (intel_miptree_needs_fake_etc(brw, mt)) {
> + assert(mt->shadow_mt);
> + mt = mt->shadow_mt;
> }
>
> const int surf_index = surf_offset - &brw->wm.base.surf_offset[0];
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 479188fd1c8..1643ce2eeb2 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -57,6 +57,11 @@ static void *intel_miptree_map_raw(struct brw_context *brw,
> GLbitfield mode);
>
> static void intel_miptree_unmap_raw(struct intel_mipmap_tree *mt);
> +static void intel_miptree_update_etc_shadow(struct brw_context *brw,
> + struct intel_mipmap_tree *mt,
> + unsigned int level,
> + unsigned int slice,
> + int level_w, int level_h);
We can delete this declaration in the next patch.
>
> static bool
> intel_miptree_supports_mcs(struct brw_context *brw,
> @@ -687,10 +692,8 @@ miptree_create(struct brw_context *brw,
> if (devinfo->gen < 6 && _mesa_is_format_color_format(format))
> tiling_flags &= ~ISL_TILING_Y0_BIT;
>
> - mesa_format mt_fmt;
> - if (_mesa_is_format_color_format(format)) {
> - mt_fmt = intel_lower_compressed_format(brw, format);
> - } else {
> + mesa_format mt_fmt = format;
> + if (!_mesa_is_format_color_format(format)) {
> /* Fix up the Z miptree format for how we're splitting out separate
> * stencil. Gen7 expects there to be no stencil bits in its depth
> buffer.
> */
We might as well simplify the assignment of mt_fmt so that we're only
assigning it the value of `format` once.
> @@ -707,6 +710,25 @@ miptree_create(struct brw_context *brw,
> if (mt == NULL)
> return NULL;
>
> + if (intel_miptree_needs_fake_etc(brw, mt)) {
> + mesa_format decomp_format = intel_lower_compressed_format(brw, format);
> + mt->etc_format = format;
With this patch, the etc_format field is made redundant. In a separate
patch, we could actually delete it and replace its uses with the new
information we have about the miptree object.
> + mt->shadow_mt = make_surface(brw, target, decomp_format, first_level,
> + last_level, width0, height0, depth0,
> + num_samples, tiling_flags,
> + mt_surf_usage(mt_fmt),
^
It doesn't affect the output right now, but this should probably be
decomp_format.
> + alloc_flags, 0, NULL);
> +
> + if (mt->shadow_mt == NULL) {
> + intel_miptree_release(&mt);
> + return NULL;
> + }
> +
> + mt->shadow_mt->etc_format = MESA_FORMAT_NONE;
> + } else {
> + mt->etc_format = MESA_FORMAT_NONE;
> + }
> +
> if (needs_separate_stencil(brw, mt, format)) {
> mt->stencil_mt =
> make_surface(brw, target, MESA_FORMAT_S_UINT8, first_level,
> last_level,
> @@ -719,9 +741,6 @@ miptree_create(struct brw_context *brw,
> }
> }
>
> - mt->etc_format = (_mesa_is_format_color_format(format) && mt_fmt !=
> format) ?
> - format : MESA_FORMAT_NONE;
> -
> if (!(flags & MIPTREE_CREATE_NO_AUX))
> intel_miptree_choose_aux_usage(brw, mt);
>
> @@ -2426,8 +2445,11 @@ intel_miptree_finish_write(struct brw_context *brw,
>
> switch (mt->aux_usage) {
> case ISL_AUX_USAGE_NONE:
> - if (mt->format == MESA_FORMAT_S_UINT8 && devinfo->gen <= 7)
> + if (mt->format == MESA_FORMAT_S_UINT8 && devinfo->gen <= 7) {
> + mt->shadow_needs_update = true;
> + } else if (intel_miptree_has_etc_shadow(brw, mt)) {
> mt->shadow_needs_update = true;
> + }
> break;
>
> case ISL_AUX_USAGE_MCS:
> @@ -3454,61 +3476,6 @@ intel_miptree_map_s8(struct brw_context *brw,
> map->unmap = intel_miptree_unmap_s8;
> }
>
> -static void
> -intel_miptree_unmap_etc(struct brw_context *brw,
> - struct intel_mipmap_tree *mt,
> - struct intel_miptree_map *map,
> - unsigned int level,
> - unsigned int slice)
> -{
> - uint32_t image_x;
> - uint32_t image_y;
> - intel_miptree_get_image_offset(mt, level, slice, &image_x, &image_y);
> -
> - image_x += map->x;
> - image_y += map->y;
> -
> - uint8_t *dst = intel_miptree_map_raw(brw, mt, GL_MAP_WRITE_BIT)
> - + image_y * mt->surf.row_pitch_B
> - + image_x * mt->cpp;
> -
> - if (mt->etc_format == MESA_FORMAT_ETC1_RGB8)
> - _mesa_etc1_unpack_rgba8888(dst, mt->surf.row_pitch_B,
> - map->ptr, map->stride,
> - map->w, map->h);
> - else
> - _mesa_unpack_etc2_format(dst, mt->surf.row_pitch_B,
> - map->ptr, map->stride,
> - map->w, map->h, mt->etc_format, true);
> -
> - intel_miptree_unmap_raw(mt);
> - free(map->buffer);
> -}
> -
> -static void
> -intel_miptree_map_etc(struct brw_context *brw,
> - struct intel_mipmap_tree *mt,
> - struct intel_miptree_map *map,
> - unsigned int level,
> - unsigned int slice)
> -{
> - assert(mt->etc_format != MESA_FORMAT_NONE);
> - if (mt->etc_format == MESA_FORMAT_ETC1_RGB8) {
> - assert(mt->format == MESA_FORMAT_R8G8B8X8_UNORM);
> - }
> -
> - assert(map->mode & GL_MAP_WRITE_BIT);
> - assert(map->mode & GL_MAP_INVALIDATE_RANGE_BIT);
> -
> - intel_miptree_access_raw(brw, mt, level, slice, true);
> -
> - map->stride = _mesa_format_row_stride(mt->etc_format, map->w);
> - map->buffer = malloc(_mesa_format_image_size(mt->etc_format,
> - map->w, map->h, 1));
> - map->ptr = map->buffer;
> - map->unmap = intel_miptree_unmap_etc;
> -}
> -
> /**
> * Mapping functions for packed depth/stencil miptrees backed by real
> separate
> * miptrees for depth and stencil.
> @@ -3781,9 +3748,6 @@ intel_miptree_map(struct brw_context *brw,
>
> if (mt->format == MESA_FORMAT_S_UINT8) {
> intel_miptree_map_s8(brw, mt, map, level, slice);
> - } else if (mt->etc_format != MESA_FORMAT_NONE &&
> - !(mode & BRW_MAP_DIRECT_BIT)) {
> - intel_miptree_map_etc(brw, mt, map, level, slice);
> } else if (mt->stencil_mt && !(mode & BRW_MAP_DIRECT_BIT)) {
> intel_miptree_map_depthstencil(brw, mt, map, level, slice);
> } else if (use_intel_mipree_map_blit(brw, mt, map)) {
> @@ -3816,6 +3780,7 @@ intel_miptree_unmap(struct brw_context *brw,
> unsigned int slice)
> {
> struct intel_miptree_map *map = mt->level[level].slice[slice].map;
> + int level_w, level_h;
>
> assert(mt->surf.samples == 1);
>
> @@ -3825,10 +3790,21 @@ intel_miptree_unmap(struct brw_context *brw,
> DBG("%s: mt %p (%s) level %d slice %d\n", __func__,
> mt, _mesa_get_format_name(mt->format), level, slice);
>
> + level_w = minify(mt->surf.phys_level0_sa.width,
> + level - mt->first_level);
> + level_h = minify(mt->surf.phys_level0_sa.height,
> + level - mt->first_level);
> +
> if (map->unmap)
> map->unmap(brw, mt, map, level, slice);
>
> intel_miptree_release_map(mt, level, slice);
> +
> + if (intel_miptree_has_etc_shadow(brw, mt) && mt->shadow_needs_update) {
> + mt->shadow_needs_update = false;
> + intel_miptree_update_etc_shadow(brw, mt, level, slice, level_w,
> + level_h);
> + }
> }
>
> enum isl_surf_dim
> @@ -3943,3 +3919,71 @@ intel_miptree_get_clear_color(const struct
> gen_device_info *devinfo,
> return mt->fast_clear_color;
> }
> }
> +
> +static void
> +intel_miptree_update_etc_shadow(struct brw_context *brw,
> + struct intel_mipmap_tree *mt,
> + unsigned int level,
> + unsigned int slice,
> + int level_w,
> + int level_h)
> +{
> + struct intel_mipmap_tree *smt;
> + ptrdiff_t etc_stride, shadow_stride;
> + GLbitfield etc_mode, shadow_mode;
> + void *mptr, *sptr;
> +
> + assert(intel_miptree_has_etc_shadow(brw, mt));
> +
> + smt = mt->shadow_mt;
> +
> + etc_mode = GL_MAP_READ_BIT;
> + shadow_mode = GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT;
I think it's simpler to put these constants in the function calls.
> +
> + intel_miptree_map(brw, mt, level, slice, 0, 0, level_w, level_h,
> + etc_mode, &mptr, &etc_stride);
> + intel_miptree_map(brw, smt, level, slice, 0, 0, level_w, level_h,
> + shadow_mode, &sptr, &shadow_stride);
> +
> + if (mt->format == MESA_FORMAT_ETC1_RGB8) {
> + _mesa_etc1_unpack_rgba8888(sptr, shadow_stride, mptr, etc_stride,
> + level_w, level_h);
> + } else {
> + /* destination and source images must have the same swizzle */
> + bool is_bgra = (smt->format == MESA_FORMAT_B8G8R8A8_SRGB);
> + _mesa_unpack_etc2_format(sptr, shadow_stride, mptr, etc_stride,
> + level_w, level_h, mt->format, is_bgra);
> + }
> +
> + intel_miptree_unmap(brw, mt, level, slice);
> + intel_miptree_unmap(brw, smt, level, slice);
> +}
> +
> +void
> +intel_miptree_update_etc_shadow_levels(struct brw_context *brw,
> + struct intel_mipmap_tree *mt)
> +{
> + struct intel_mipmap_tree *smt;
> + int num_slices;
> +
> + assert(mt);
> + assert(mt->surf.size_B > 0);
> + assert(intel_miptree_has_etc_shadow(brw, mt));
> +
> + smt = mt->shadow_mt;
> + num_slices = smt->surf.logical_level0_px.array_len;
This function and the one above it splits variable declarations and
assignments sometimes and combines them at other times. I'd prefer they
be more consistent by combining all declarations and assignments.
-Nanley
> +
> + for (int level = smt->first_level; level <= smt->last_level; level++) {
> + int level_w = minify(smt->surf.logical_level0_px.width,
> + level - smt->first_level);
> + int level_h = minify(smt->surf.logical_level0_px.height,
> + level - smt->first_level);
> +
> + for (unsigned int slice = 0; slice < num_slices; slice++) {
> + intel_miptree_update_etc_shadow(brw, mt, level, slice, level_w,
> + level_h);
> + }
> + }
> +
> + mt->shadow_needs_update = false;
> +}
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index 1a7507023a1..752aeaaf9b7 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -301,6 +301,8 @@ struct intel_mipmap_tree
> *
> * This miptree may be used for:
> * - Stencil texturing (pre-BDW) as required by GL_ARB_stencil_texturing.
> + * - To store the decompressed ETC/EAC data in case we emulate the ETC
> + * compression on Gen 7 or earlier GPUs.
> */
> struct intel_mipmap_tree *shadow_mt;
> bool shadow_needs_update;
> @@ -730,6 +732,28 @@ isl_memcpy_type
> intel_miptree_get_memcpy_type(mesa_format tiledFormat, GLenum format, GLenum
> type,
> uint32_t *cpp);
>
> +static inline bool
> +intel_miptree_needs_fake_etc(struct brw_context *brw,
> + struct intel_mipmap_tree *mt)
> +{
> + const struct gen_device_info *devinfo = &brw->screen->devinfo;
> + bool is_etc = _mesa_is_format_etc2(mt->format) ||
> + (mt->format == MESA_FORMAT_ETC1_RGB8);
> +
> + return devinfo->gen < 8 && !devinfo->is_baytrail && is_etc;
> +}
> +
> +static inline bool
> +intel_miptree_has_etc_shadow(struct brw_context *brw,
> + struct intel_mipmap_tree *mt)
> +{
> + return intel_miptree_needs_fake_etc(brw, mt) && mt->shadow_mt;
> +}
> +
> +void
> +intel_miptree_update_etc_shadow_levels(struct brw_context *brw,
> + struct intel_mipmap_tree *mt);
> +
> #ifdef __cplusplus
> }
> #endif
> --
> 2.20.1
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
