<div dir="auto">On Sun, Feb 10, 2019 at 11:31:05PM +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) > --- > .../drivers/dri/i965/brw_wm_surface_state.c | 5 +- > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 185 +++++++++++------- > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 24 +++ > 3 files changed, 147 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 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); > + 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..e50db649a23 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 decomp_format = intel_lower_compressed_format(brw, format); > + mt->etc_format = format; > + 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), > + 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,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); > + Oh okay, phys_level0_sa is equal to logical_level0_px aligned up to the block size (I misread it as a divided by earlier). This works fine. > 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); For the issue mentioned earlier about marking the shadow as updated, I'd prefer if it was fixed in this patch. As things are now, intel_miptree_update_etc_shadow_levels() is broken until the next patch. The solution I had in mind was to modify this patch in two places: A) We include the following line here: mt->shadow_needs_update = false; > + } > } > > enum isl_surf_dim > @@ -3943,3 +3918,81 @@ 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; B) We remove the line above. > + smt = mt->shadow_mt; > + > + etc_mode = GL_MAP_READ_BIT; > + shadow_mode = GL_MAP_WRITE_BIT | GL_MAP_INVALIDATE_RANGE_BIT; > + > + 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; > + 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++) > + { ^ The coding style is to have the brace on the same line as the for. > + 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(smt->surf.logical_level0_px.width, > + level - smt->first_level); > + level_h = minify(smt->surf.logical_level0_px.height, > + level - smt->first_level); This function still has the level_w/h bug mentioned earlier. Although it's fixed in the next patch, we generally try to avoid introducing functions with bugs. > + } > + How about a simpler fix that looks something like this: smt = mt->shadow_mt; 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++) { 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); intel_miptree_update_etc_shadow(brw, mt, level, slice, level_w, level_h); } } -Nanley > + 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 > </div>