[Mesa-dev] [PATCH] i965: consider a 'base level' when calculating width0, height0, depth0

[Kenneth Graunke]

On Thursday, October 11, 2018 12:12:38 PM PDT Kenneth Graunke wrote:
> On Thursday, October 11, 2018 11:58:40 AM PDT Kenneth Graunke wrote:
> > On Tuesday, October 2, 2018 9:16:01 AM PDT asimiklit.work at gmail.com wrote:
> > > From: Andrii Simiklit <andrii.simiklit at globallogic.com>
> > > 
> > > I guess that when we calculating the width0, height0, depth0
> > > to use for function 'intel_miptree_create' we need to consider
> > > the 'base level' like it is done in the 'intel_miptree_create_for_teximage'
> > > function.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107987
> > > Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
> > > ---
> > >  .../drivers/dri/i965/intel_tex_validate.c     | 26 ++++++++++++++++++-
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > I believe this patch is correct - we're assembling things into a new
> > miptree, which we start at level 0 - so we need the sizes for level 0.
> > 
> > Alternatively, we might be able to pass validate_first_level instead
> > of 0 when calling intel_miptree_create, to make one that's only good
> > up until the new base...and have to re-assemble it the next time they
> > change the base.  It would save memory potentially.  But more copies.
> > I don't have a strong preference which is better.
> > 
> > Please do make a Piglit or dEQP test for this.
> > 
> > Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> Sorry, withdrawing my review. :(  Chris Forbes pointed out on IRC that
> your reproducer case is backwards:
> 
> miplevel 0 - 1x1
> miplevel 1 - 2x2
> miplevel 2 - 4x4
> 
> That's upside down.  A proper miptree would have the base be largest:
> 
> miplevel 0 - 4x4
> miplevel 1 - 2x2
> miplevel 2 - 1x1
> 
> So, yes, I could see this tripping an assert...but such a crazy texture
> will never be mipmap complete.  If they're expecting mipmapping, then
> it seems like they should get a fallback black texture (which normally
> happens for incomplete textures).  If not, maybe they should get a
> single miplevel?  Either way, seems like we should detect insanity and
> bail, rather than change size calculations for the normal sane case.
> 

So...looked at this again.  I'm not sure why upside-down matters.

At DrawArrays time, we have a single miplevel (base = 2), and are trying
to put that single miplevel's image into a miptree.  We do properly
ignore levels 0..1 as they're beyond the base.

We appear to use level 0 as the actual base, and want to store our
single level at level 2.  Other places (TexImage) seem to work that way
too.

But, we're creating the miplevel with level 0 as the base, but where
level 0 has the dimensions of level 2.  This doesn't work.  And your
patch fixes that.

I tried making the actual base of the unified tree be level 2, rather
than level 0...so that the BaseLevel is the actual base...but tons of
things broke.

So, back to Reviewed-by.  I think once we get a Piglit test, I'm happy
to land this patch.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181019/e356abe0/attachment.sig>