<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 30, 2017 at 10:37 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Oct 27, 2017 at 05:14:16PM -0700, Jason Ekstrand wrote: > On Fri, Oct 27, 2017 at 3:16 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > On Fri, Oct 27, 2017 at 12:52:30PM -0700, Jason Ekstrand wrote: > > > On Fri, Oct 27, 2017 at 12:24 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> > > > wrote: > > > > > > > Only use CCS_E to render to a texture that is CCS_E-compatible with the > > > > original texture's miptree (linear) format. This prevents render > > > > operations from writing data that can't be decoded with the original > > > > miptree format. > > > > > > > > On Gen10, with the new CCS_E-enabled formats handled, this enables the > > > > driver to pass the arb_texture_view-rendering-formats piglit test. > > > > > > > > Cc: <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> > > > > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > > > --- > > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 28 > > > > +++++++++++++++++++++++++-- > > > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > index a850f4d17b..59c57c227b 100644 > > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > > @@ -241,6 +241,27 @@ intel_miptree_supports_hiz(const struct > > brw_context > > > > *brw, > > > > } > > > > } > > > > > > > > +/** > > > > + * Return true if the format that will be used to access the miptree > > is > > > > + * CCS_E-compatible with the miptree's linear/non-sRGB format. > > > > + * > > > > + * Why use the linear format? Well, although the miptree may be > > specified > > > > with > > > > + * an sRGB format, the usage of that color space/format can be > > toggled. > > > > Since > > > > + * our HW tends to support more linear formats than sRGB ones, we use > > this > > > > + * format variant for check for CCS_E compatibility. > > > > + */ > > > > +static bool > > > > +format_ccs_e_compat_with_miptree(const struct gen_device_info > > *devinfo, > > > > + const struct intel_mipmap_tree *mt, > > > > + enum isl_format access_format) > > > > +{ > > > > + assert(mt->aux_usage == ISL_AUX_USAGE_CCS_E); > > > > + > > > > + mesa_format linear_format = _mesa_get_srgb_format_linear( > > mt->format); > > > > + enum isl_format isl_format = brw_isl_format_for_mesa_ > > > > format(linear_format); > > > > + return isl_formats_are_ccs_e_compatible(devinfo, isl_format, > > > > access_format); > > > > +} > > > > + > > > > static bool > > > > intel_miptree_supports_ccs_e(struct brw_context *brw, > > > > const struct intel_mipmap_tree *mt) > > > > @@ -2662,8 +2683,11 @@ intel_miptree_render_aux_usage(struct > > brw_context > > > > *brw, > > > > return mt->mcs_buf ? ISL_AUX_USAGE_CCS_D : ISL_AUX_USAGE_NONE; > > > > > > > > case ISL_AUX_USAGE_CCS_E: { > > > > - /* If the format supports CCS_E, then we can just use it */ > > > > - if (isl_format_supports_ccs_e(&brw->screen->devinfo, > > > > render_format)) > > > > + /* If the format supports CCS_E and is compatible with the > > miptree, > > > > + * then we can use it. > > > > + */ > > > > + if (format_ccs_e_compat_with_miptree(&brw->screen->devinfo, > > > > + mt, render_format)) > > > > > > > > > > You don't need the helper if you just use mt->surf.format. That's what > > > can_texture_with_ccs does. > > > > > > > > > > Isn't using mt->surf.format making the code more restrictive than > > necessary? That field may give you the sRGB format of the surface, but > > the helper would always give you the linear variant. Therefore the > > helper allows for CCS_E to be used in more cases. > > > > You're right. I completely missed that. In that case, we probably want to > use your helper for can_texture_with_ccs as well. Maybe two patches: one > which adds the helper and makes can_texture_with_ccs use it and a second to > add the render target stuff? Or you can keep this one as-is and do > can_texture_with_ccs as a follow-on, I don't really care. </div></div>I'm assuming I have your Rb? I'm thinking of keeping this one as-is. I tried modifying can_texture_with_ccs to use the helper before sending the patch out, but it caused one GLES3.1 test to fail and I couldn't figure out why (on jenkins, nchery build#249). dEQP-GLES31.functional.srgb_texture_decode.skip_decode.srgba8.texel_fetch Since this patch is needed for correctness and the other is more of a performance enhancement, I figured we could get to it later. Any ideas would be appreciated though. </blockquote></div></div><div class="gmail_extra"> </div><div class="gmail_extra">Yuck... Yeah, I think this is probably fine. It might be good to move the format_ccs_e_compat_with_miptree helper up a little higher so we can use it with texturing and maybe leave a Todo. But yeah, R-B.</div><div class="gmail_extra"> </div><div class="gmail_extra">--Jason </div></div>