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

[Chad Versace]

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.

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;

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

> 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