[Mesa-dev] [PATCH v4 05/18] intel/isl: Add support to emit clear value address.

[Pohjolainen, Topi]

On Tue, Mar 27, 2018 at 11:01:34AM -0700, Jason Ekstrand wrote:
> On Tue, Mar 27, 2018 at 4:31 AM, Pohjolainen, Topi <
> topi.pohjolainen at gmail.com> wrote:
> 
> > On Thu, Mar 08, 2018 at 08:48:58AM -0800, Rafael Antognolli wrote:
> > > gen10 can emit the clear color by setting it on a buffer somewhere, and
> > > then adding only the address to the surface state.
> > >
> > > This commit add support for that on isl_surf_fill_state, and if that is
> > > requested, skip setting the clear value itself.
> > >
> > > v2: Add assert to make sure we are at least on gen10.
> > >
> > > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > > ---
> > >  src/intel/isl/isl.h               |  9 +++++++++
> > >  src/intel/isl/isl_surface_state.c | 18 ++++++++++++++----
> > >  2 files changed, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> > > index 2edf0522e32..c50b78d4701 100644
> > > --- a/src/intel/isl/isl.h
> > > +++ b/src/intel/isl/isl.h
> > > @@ -1307,6 +1307,15 @@ struct isl_surf_fill_state_info {
> > >      */
> > >     union isl_color_value clear_color;
> > >
> > > +   /**
> > > +    * Send only the clear value address
> > > +    *
> > > +    * If set, we only pass the clear address to the GPU and it will
> > fetch it
> > > +    * from wherever it is.
> > > +    */
> > > +   bool use_clear_address;
> > > +   uint64_t clear_address;
> > > +
> > >     /**
> > >      * Surface write disables for gen4-5
> > >      */
> > > diff --git a/src/intel/isl/isl_surface_state.c
> > b/src/intel/isl/isl_surface_state.c
> > > index 32a5429f2bf..bff9693f02d 100644
> > > --- a/src/intel/isl/isl_surface_state.c
> > > +++ b/src/intel/isl/isl_surface_state.c
> > > @@ -637,11 +637,21 @@ isl_genX(surf_fill_state_s)(const struct
> > isl_device *dev, void *state,
> > >  #endif
> > >
> > >     if (info->aux_usage != ISL_AUX_USAGE_NONE) {
> > > +      if (info->use_clear_address) {
> > > +#if GEN_GEN >= 10
> > > +         s.ClearValueAddressEnable = true;
> >
> > In order to make sampler working, I had to add:
> >
> > #if GEN_GEN >= 11
> >       /* From the Bspec:
> >        *
> >        *  Enables Pixel backend hw to convert clear values into native
> > format
> >        *  and write back to clear address, so that display and sampler can
> > use
> >        *  the converted value for resolving fast cleared RTs.
> >        */
> >       s.ClearColorConversionEnable = s.ClearValueAddressEnable;
> >
> 
> Yeah... That's a fun bit...  I think we want to be careful with that bit
> since it causes a write to the CLEAR_COLOR state as part of the draw.  It
> *may* be safe to always set it but I'm not convinced.  We should probably
> add another bit to isl_surf_init_info and only set it from BLORP when doing
> a fast-clear.  Another option would be to make BLORP just do the conversion
> in software and write the clear value out manually.  In either case, yes,
> we need to do something for ICL.
> 
> Given that ICL fast-clears aren't working yet, I'd be a fan (if no one's
> opposed) to plumbing that through as a follow-on.

You are correct that we should be careful. Setting it like this
unconditionally breaks at least:

ext_framebuffer_multisample-accuracy 2 color

I'll give a spin to your suggestion of setting it only in blorp clear.

> 
> 
> > #endif
> >
> >
> > That along with another tweak in one of the later patches fixes at least
> > these
> > two on ICL:
> >
> > fbo-clearmipmap -auto -fbo
> > fcc-read-after-clear sample tex -fbo -auto
> >
> > > +         s.ClearValueAddress = info->clear_address;
> > > +#else
> > > +         unreachable("Gen9 and earlier do not support indirect clear
> > colors");
> > > +#endif
> > > +      }
> > >  #if GEN_GEN >= 9
> > > -      s.RedClearColor = info->clear_color.u32[0];
> > > -      s.GreenClearColor = info->clear_color.u32[1];
> > > -      s.BlueClearColor = info->clear_color.u32[2];
> > > -      s.AlphaClearColor = info->clear_color.u32[3];
> > > +      if (!info->use_clear_address) {
> > > +         s.RedClearColor = info->clear_color.u32[0];
> > > +         s.GreenClearColor = info->clear_color.u32[1];
> > > +         s.BlueClearColor = info->clear_color.u32[2];
> > > +         s.AlphaClearColor = info->clear_color.u32[3];
> > > +      }
> > >  #elif GEN_GEN >= 7
> > >        /* Prior to Sky Lake, we only have one bit for the clear color
> > which
> > >         * gives us 0 or 1 in whatever the surface's format happens to be.
> > > --
> > > 2.14.3
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >