[Mesa-dev] [PATCH 11/11] i965: Fix textureSize for Lod > 0 with non-mipmap filters

[Iago Toral]

Hi Ben,

On Fri, 2015-04-10 at 15:34 -0700, Ben Widawsky wrote:
> On Tue, Feb 10, 2015 at 04:40:48PM +0100, Eduardo Lima Mitev wrote:
> > From: Iago Toral Quiroga <itoral at igalia.com>
> > 
> > Currently, when the MinFilter is GL_LINEAR or GL_NEAREST we hide the
> > actual miplevel count from the hardware (and we avoid re-creating
> > the miptree structure with all the levels), since we don't expect
> > levels other than the base level to be needed. Unfortunately,
> > GLSL's textureSize() function is an exception to this rule. This
> > function takes a lod parameter that we need to use to return the
> > size of the appropriate miplevel (if it exists). The spec only
> > requires that the miplevel exists, so even if the sampler is
> > configured with a linear or nearest MinFilter, as far as the user
> > has uploaded miplevels for the texture, textureSize() should return
> > the appropriate sizes.
> > 
> > This patch fixes this by exposing the actual miplevel count for all
> > sampling engine textures while keeping the original implementation
> > for render targets (for render targets textures we do not provide
> > the miplevel count but the actual LOD we are wrting to, so we
> > want to make sure that we make this the base level).
> > 
> > Fixes 28 dEQP tests in the following category:
> > dEQP-GLES3.functional.shaders.texture_functions.texturesize.*
> > ---
> >  src/mesa/drivers/dri/i965/intel_tex_validate.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/intel_tex_validate.c b/src/mesa/drivers/dri/i965/intel_tex_validate.c
> > index 0bf0393..06aeca6 100644
> > --- a/src/mesa/drivers/dri/i965/intel_tex_validate.c
> > +++ b/src/mesa/drivers/dri/i965/intel_tex_validate.c
> > @@ -47,8 +47,10 @@ intel_update_max_level(struct intel_texture_object *intelObj,
> >  {
> >     struct gl_texture_object *tObj = &intelObj->base;
> >  
> > -   if (sampler->MinFilter == GL_NEAREST ||
> > -       sampler->MinFilter == GL_LINEAR) {
> > +   if (!tObj->_MipmapComplete ||
> > +       (tObj->_RenderToTexture &&
> > +        (sampler->MinFilter == GL_NEAREST ||
> > +         sampler->MinFilter == GL_LINEAR))) {
> >        intelObj->_MaxLevel = tObj->BaseLevel;
> >     } else {
> >        intelObj->_MaxLevel = tObj->_MaxLevel;
> 
> Forgive my ignorance on this subject in advance...
> 
> I don't know if _MipmapComplete is correct here. Looking at
> _mesa_test_texobj_completeness, it /seems/ like almost all the cases which would
> mark the texture as !_MipmapComplete are errors, but reading the comments for
> incomplete() suggests to me there are potentially valid cases where you could
> use textureSize(), but have an incomplete mipmap. IN OTHER WORDS... it seems to
> me that you may want something other than tObj->BaseLevel even when
> !tObj->_MipmapComplete. Clarification would be greatly appreciated.

That was necessary to fix a piglit test regression. I don't remember
what piglit test it was, sorry :(, but I do remember the problem:
The test uploaded a mipmap-complete texture first, did some tests with
it and then it would upload level 0 again with a _different_ texture
size (which would make the texture mipmap incomplete again because all
levels other than the base would be incorrect at this point). In this
scenario only the base level is valid and we want to make sure that we
only upload that. Not doing this would led to crash in the driver at
some point later on.

Looking at the comments in incomplete(), cases 1 and 3 seem like errors
to me, and case 2  is exactly the case triggered by the piglit test I
mention above (in which we want only the base level uploaded). I think
_MipmapComplete can only be TRUE when levels other than the base level
are valid.

Make sense?

> I assume the use of _MipmapComplete was what required adding the bit about
> tObj->_RenderToTexture (because certain textures are considered complete even
> when they're not??). I don't know enough to assert correctness for that though.
> Would be nice if you could dumb this down for me as well.

Not really, the reason to add _RenderToTexture is different, it is
related to this paragraph from the commit log:

"This patch fixes this by exposing the actual miplevel count for all
sampling engine textures while keeping the original implementation for
render targets (for render targets textures we do not provide
the miplevel count but the actual LOD we are wrting to, so we
want to make sure that we make this the base level)."

So what I found is that for render targets the driver wouldn't use
_MaxLevel  to set the miplevel count, it would use it to select the LOD
we write to. So in this case we wanted to preserve the original behavior
to make sure that we only write to the base level when linear filters
are enabled. If we don't do this there are regressions.

These are the relevant source code and PRM references:

File gen7_wm_surface_state.c, function gen7_update_texture_surface():

surf[5] = (SET_FIELD(GEN7_MOCS_L3, GEN7_SURFACE_MOCS) |
           SET_FIELD(tObj->MinLevel + tObj->BaseLevel - mt->first_level,
GEN7_SURFACE_MIN_LOD) |
           /* mip count */
           (intelObj->_MaxLevel - tObj->BaseLevel));

And from the IVB PRM, vol4, part1, RENDER_SURFACE_STATE:

Dword 5, bits 3:0, MIP Count / LOD:

"For Sampling Engine and Typed Surfaces:This field indicates the number
of MIP levels allowed to be accessed starting at Surface Min LOD, which
must be less than or equal to the number of MIP levels actually stored
in memory for this surface. Force the mip map access to be between the
mipmap specified by the integer bits of the Min LOD and the ceiling of
the value specified here. For Render Target Surfaces:This field defines
the MIP level that is currently being rendered into. This is the
absolute MIP level on the surface and is not relative to the Surface 
Min LOD field, which is ignored for render target surfaces."

With this information, does the patch look good to you?

> With a valid explanation of how tObj->_MipmapComplete and tObj->_RenderToTexture
> are indeed the correct thing always, it's:
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net> (with the ignorance caveat)
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev