[Mesa-dev] [PATCH 2/2] i965: Check CCS_E compatibility for texture view rendering

[Nanley Chery]

On Tue, Oct 31, 2017 at 02:15:43PM -0700, Jason Ekstrand wrote:
> On Mon, Oct 30, 2017 at 10:37 AM, Nanley Chery <nanleychery at gmail.com>
> wrote:
> 
> > On Fri, Oct 27, 2017 at 05:14:16PM -0700, Jason Ekstrand wrote:
> > > On Fri, Oct 27, 2017 at 3:16 PM, Nanley Chery <nanleychery at gmail.com>
> > 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 <
> > nanleychery at gmail.com>
> > > > > 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: <mesa-stable at lists.freedesktop.org>
> > > > > > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > > > > > ---
> > > > > >  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.
> >
> > 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.
> >
> 
> 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.
> 

Thanks! It's already higher than can_texture_with_ccs, so I'll just add
the Todo.

> --Jason