On Tue, Nov 21, 2017 at 10:56 AM, Chad Versace <[email protected]> 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" <[email protected]> > > Cc: Kenneth Graunke <[email protected]> > > --- > > 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 > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
