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

[Chad Versace]

On Tue 21 Nov 2017, Jason Ekstrand wrote:
> On Tue, Nov 21, 2017 at 10:56 AM, Chad Versace <[1]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" <[2]mesa-stable at lists.freedesktop.org>
>     > Cc: Kenneth Graunke <[3]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.

Ok. That makes sense.

>  
> 
>     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.

Even if this patch degrades some corner-case fast clears to slow clears,
the fixes in intel_mipmap_tree.c are worth it.

Patch 1/4 is
Reviewed-by: Chad Versace <chadversary at chromium.org>