<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 21, 2017 at 10:56 AM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed 08 Nov 2017, Jason Ekstrand wrote: <div><div class="h5">> 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" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> > Cc: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > --- > 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; </div></div>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. </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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; </blockquote><div> </div><div>Ugh... Fortunately, I think that only happens in some rather obscure render-to-texture cases. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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. </blockquote><div> </div><div>I think it's the same answer. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>