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

[Nanley Chery]

On Fri, Feb 08, 2019 at 12:55:20PM +0200, Eleni Maria Stea wrote:
> Hi Nanley,
> 
> On Thu, 7 Feb 2019 15:46:29 -0800
> Nanley Chery <nanleychery at gmail.com> wrote:
>  >  
> > > @@ -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?
> 
> After a second thought, I believe that this change wasn't unnecessary.
> There is a problem if we remove it:
> 
> When we generate mipmaps we need to update the shadow for each level.
> As the update is done per level during unmap, if we remove the call we
> end-up with the first level correctly updated but all the others empty.
> 
> An example:
> git clone https://github.com/hikiko/test-compression.git
> make
> ./test compressed/full.tex
> 

That's a nice test :)

> This test loads dumped compressed mipmap levels from the full.tex and
> displays them, if you run it with the per level update inside the unmap
> you will see all the mipmap levels. Without, you will see only the
> first, like here: https://imgur.com/a/VvS0CYC
> 
> Do you have any suggestion on how I could bypass this problem?
> 

Yes. The cause of this problem is that after
intel_miptree_update_etc_shadow_levels() calls
intel_miptree_update_etc_shadow() on the first level, the miptree is
marked as not needing an update. Therefore subsequent calls to
intel_miptree_update_etc_shadow() return early without updating other
levels. One way to fix this is to move the responsibility of marking the
miptree as updated from intel_miptree_update_etc_shadow() to it's
callers.

I also found another bug. I'll add a comment on the problematic patch.

> Thanks again,
> Eleni
> 
> 
> 
>