[Mesa-dev] [PATCH v3 2/5] i965: Faking the ETC2 compression on Gen < 8 GPUs using two miptrees.

Eleni Maria Stea estea at igalia.com
Fri Feb 8 07:39:03 UTC 2019


Hello,

On Thu, 7 Feb 2019 15:46:29 -0800
Nanley Chery <nanleychery at gmail.com> wrote:


> > -              !(mode & BRW_MAP_DIRECT_BIT)) {
> > +              !(mode & BRW_MAP_DIRECT_BIT) &&
> > +              !(intel_miptree_needs_fake_etc(brw, mt))) {
> >        intel_miptree_map_etc(brw, mt, map, level, slice);  
> 
> Out of curiosity, is there any reason you wait until patch 5 to delete
> this case?

No, I just removed this lines together with the unreached map/unmap_etc
functions. I will move the change to this patch.

> 
> >     } else if (mt->stencil_mt && !(mode & BRW_MAP_DIRECT_BIT)) {
> >        intel_miptree_map_depthstencil(brw, mt, map, level, slice);
> > @@ -3816,6 +3839,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 +3849,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, the change in this function becomes
> unnecessary. Is there any reason you're leaving it around?

Right, if we force the update before the rendering, we don't need to
copy the data during the unmap. I will remove it, sorry I dismissed
it in the previous email.
 
> >  }
> >  
> >  enum isl_surf_dim
> > @@ -3943,3 +3977,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;
> > +   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++)
> > +   {
> > +      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.logical_level0_px.width,
> > +                       level - mt->first_level);
> > +      level_h = minify(mt->surf.logical_level0_px.height,
> > +                       level - mt->first_level);  
> 
> This function goes from accessing the smt to the mt for the
> logical_level0_px field. It does work, but it is a little surprising.

I will fix that too, it might have worked because the trees have the
same logical_level0_px w, h, but yes the correct thing is to use the
smt here :)

> 
> > +   }
> > +
> > +   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;  
> 
> This function does the same work as the various
> mt->etc_format != MESA_FORMAT_NONE checks we have in i965, but I
> prefer the way you've implemented it here.

Ok, thanks a lot for the review, I'm going to fix the patches and send
the new version.

Regards,
Eleni


More information about the mesa-dev mailing list