[Mesa-dev] [PATCH 1/4] i965/miptree: Loosen the format check in miptree_match_image

[Jason Ekstrand]

On Tue, Nov 21, 2017 at 10:56 AM, Chad Versace <chadversary at chromium.org>
wrote:

> On Wed 08 Nov 2017, Jason Ekstrand wrote:
> > This function is used to determine when we need to re-allocate a
> > miptree.  Since we do nothing different in miptree allocation for
> > sRGB vs. linear, loosening this should be safe and may lead to less
> > copying and reallocating in some odd cases.
> >
> > Cc: "17.3" <mesa-stable at lists.freedesktop.org>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c  | 6 ++++--
> >  src/mesa/drivers/dri/i965/intel_tex.c          | 2 +-
> >  src/mesa/drivers/dri/i965/intel_tex_obj.h      | 4 ++--
> >  src/mesa/drivers/dri/i965/intel_tex_validate.c | 2 +-
> >  4 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 82f5a81..47cfccc 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -1298,7 +1298,8 @@ intel_miptree_match_image(struct
> intel_mipmap_tree *mt,
> >     if (mt->etc_format != MESA_FORMAT_NONE)
> >        mt_format = mt->etc_format;
> >
> > -   if (image->TexFormat != mt_format)
> > +   if (_mesa_get_srgb_format_linear(image->TexFormat) !=
> > +       _mesa_get_srgb_format_linear(mt_format))
> >        return false;
> >
> >     intel_get_image_dims(image, &width, &height, &depth);
> > @@ -1537,7 +1538,8 @@ intel_miptree_copy_slice(struct brw_context *brw,
> >     assert(src_layer < get_num_phys_layers(&src_mt->surf,
> >                                            src_level -
> src_mt->first_level));
> >
> > -   assert(src_mt->format == dst_mt->format);
> > +   assert(_mesa_get_srgb_format_linear(src_mt->format) ==
> > +          _mesa_get_srgb_format_linear(dst_mt->format));
> >
> >     if (dst_mt->compressed) {
> >        unsigned int i, j;
> > diff --git a/src/mesa/drivers/dri/i965/intel_tex.c
> b/src/mesa/drivers/dri/i965/intel_tex.c
> > index 65a1cb3..0650b6e 100644
> > --- a/src/mesa/drivers/dri/i965/intel_tex.c
> > +++ b/src/mesa/drivers/dri/i965/intel_tex.c
> > @@ -176,7 +176,7 @@ intel_alloc_texture_storage(struct gl_context *ctx,
> >     intel_texobj->needs_validate = false;
> >     intel_texobj->validated_first_level = 0;
> >     intel_texobj->validated_last_level = levels - 1;
> > -   intel_texobj->_Format = intel_texobj->mt->format;
> > +   intel_texobj->_Format = first_image->TexFormat;
>
> Why this line? Since all format comparisons, where appropriate, convert
> sRGB to linear pre-comparison, does it make any real difference whether
> intel_texobj->_Format is mt->format or first_image->TexFormat?
>
> It *feels* cleaner to use the incoming TexFormat here rather than
> mt->format. But I want to know if if this line was required to fix
> anything.
>

Looking at the code again, I think it is.  The way this block of code works
is that the miptree gets its format from first_image and the texobj gets
its format from the miptree.  Previously, if the format changed between
UNORM and sRGB, we would create a new miptree with the new format and the
texobj would get the new format.  With this change, we no longer create a
new miptree but we still can't let the texture object and image formats get
out of sync.


> Due to interaction with below hunk from blorp, are we now disabling fast
> clears
> in more cases than previously?
>
>         # brw_blorp.c:set_write_disables()
>         /* We store clear colors as floats or uints as needed.  If there
> are
>          * texture views in play, the formats will not properly be
> respected
>          * during resolves because the resolve operations only know about
> the
>          * miptree and not the renderbuffer.
>          */
>         if (irb->Base.Base.Format != irb->mt->format)
>            can_fast_clear = false;
>

Ugh... Fortunately, I think that only happens in some rather obscure
render-to-texture cases.


> >     return true;
> >  }
> > diff --git a/src/mesa/drivers/dri/i965/intel_tex_obj.h
> b/src/mesa/drivers/dri/i965/intel_tex_obj.h
> > index 27c18b7..526f5ce 100644
> > --- a/src/mesa/drivers/dri/i965/intel_tex_obj.h
> > +++ b/src/mesa/drivers/dri/i965/intel_tex_obj.h
> > @@ -57,8 +57,8 @@ struct intel_texture_object
> >     bool needs_validate;
> >
> >     /* Mesa format for the validated texture object. For non-views this
> > -    * will always be the same as mt->format. For views, it may differ
> > -    * since the mt is shared across views with differing formats.
> > +    * will always be the same as texObj->Image[0][0].TexFormat. For
> views, it
> > +    * may differ since the mt is shared across views with differing
> formats.
> >      */
> >     mesa_format _Format;
>
> Same question as above. Again, I feel like this is the right thing to
> do.
>

I think it's the same answer.


> > diff --git a/src/mesa/drivers/dri/i965/intel_tex_validate.c
> b/src/mesa/drivers/dri/i965/intel_tex_validate.c
> > index 2b7798c..ef7f907 100644
> > --- a/src/mesa/drivers/dri/i965/intel_tex_validate.c
> > +++ b/src/mesa/drivers/dri/i965/intel_tex_validate.c
> > @@ -174,7 +174,7 @@ intel_finalize_mipmap_tree(struct brw_context *brw,
> GLuint unit)
> >
> >     intelObj->validated_first_level = validate_first_level;
> >     intelObj->validated_last_level = validate_last_level;
> > -   intelObj->_Format = intelObj->mt->format;
> > +   intelObj->_Format = firstImage->base.Base.TexFormat,
> >     intelObj->needs_validate = false;
> >  }
> >
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171121/13b98602/attachment-0001.html>