On Sun, Feb 03, 2019 at 03:07:34PM +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)
> ---
> .../drivers/dri/i965/brw_wm_surface_state.c | 5 +-
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 133 ++++++++++++++++--
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 22 +++
> 3 files changed, 150 insertions(+), 10 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 618e2ab35bc..c2cf34aee71 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);
We can be even safer if we assert that the shadow doesn't need updating
at this time.
> + 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 0a25dfd0161..3ff36b84a5a 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);
>
> 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.
> */
> @@ -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 rgba_fmt = intel_lower_compressed_format(brw, format);
A minor nitpick, the format isn't always in rgba order but the name
implies it.
> + mt->etc_format = format;
> + mt->shadow_mt = make_surface(brw, target, rgba_fmt, first_level,
> + last_level, width0, height0, depth0,
> + num_samples, tiling_flags,
> + mt_surf_usage(mt_fmt),
> + alloc_flags, 0, NULL);
> +
> + if (mt->shadow_mt == NULL) {
> + intel_miptree_release(&mt);
> + return NULL;
> + }
> +
It might be worth documenting this new usage for the shadow_mt in the
comment above the field in the intel_mipmap_tree struct.
> + 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:
> @@ -3779,7 +3801,8 @@ 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)) {
> + !(mode & BRW_MAP_DIRECT_BIT) &&
> + !(intel_miptree_needs_fake_etc(brw, mt))) {
Isn't this case unreachable?
> 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);
> @@ -3813,6 +3836,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);
>
> @@ -3822,10 +3846,20 @@ 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) {
> + intel_miptree_update_etc_shadow(brw, mt, level, slice, level_w,
> + level_h);
> + }
With the next patch applied, aren't the changes in this function unnecessary?
> }
>
> enum isl_surf_dim
> @@ -3940,3 +3974,84 @@ 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));
> + if (!mt->shadow_needs_update)
> + return;
> +
> + mt->shadow_needs_update = false;
> + smt = mt->shadow_mt;
> +
> + etc_mode = GL_MAP_READ_BIT | BRW_MAP_DIRECT_BIT;
What does the BRW_MAP_DIRECT_BIT flag do in this case?
> + shadow_mode = GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT |
> + GL_MAP_INVALIDATE_RANGE_BIT;
shadow_mode has a redundant GL_MAP_INVALIDATE_RANGE_BIT flag.
> +
> + 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 {
> + /* set bgra to false to match the rgba swizzle */
> + bool bgra = false;
> + if (smt->format == MESA_FORMAT_B8G8R8A8_SRGB)
> + bgra = true;
You could simplify this by setting bgra equal to the predicate in the if
statement. We could avoid this special-case altogether if we change how
the formats are mapped, but this works as well.
> + _mesa_unpack_etc2_format(sptr, shadow_stride, mptr, etc_stride,
> + level_w, level_h, mt->format, 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;
> + int level_w, level_h;
> +
> + assert(mt);
> + assert(mt->surf.size_B > 0);
> +
> + assert(intel_miptree_has_etc_shadow(brw, mt));
> +
> + smt = mt->shadow_mt;
> +
> + level_w = smt->surf.logical_level0_px.width;
> + level_h = smt->surf.logical_level0_px.height;
> +
> + num_slices = smt->surf.logical_level0_px.array_len;
> +
> + for (int level = smt->first_level; level <= smt->last_level; level++)
> + {
> + for (unsigned int slice = 0; slice < num_slices; slice++) {
> + intel_miptree_update_etc_shadow(brw, mt, level, slice, level_w,
> + level_h);
> + }
> +
> + 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);
Why go from using the surfaces' logical level0 extent to it's physical
level0 extent?
We can avoid the multiple assignments to level_w/h if we reorder things
a bit.
> + }
> +
> + 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..30bc279ee42 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -730,6 +730,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 && is_etc;
This check includes Baytrail (a gen7 platform), which actually supports
ETC formats natively.
> +}
> +
> +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
