[Mesa-dev] [PATCH v5 06/19] intel/isl: Add support to emit clear value address.

[Rafael Antognolli]

On Tue, Apr 03, 2018 at 06:53:18PM +0300, Pohjolainen, Topi wrote:
> On Tue, Apr 03, 2018 at 06:50:09PM +0300, Pohjolainen, Topi wrote:
> > On Tue, Apr 03, 2018 at 08:43:54AM -0700, Rafael Antognolli wrote:
> > > On Tue, Apr 03, 2018 at 06:05:06PM +0300, Pohjolainen, Topi wrote:
> > > > On Thu, Mar 29, 2018 at 10:58:40AM -0700, 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>
> > > > > Reviewed-by: Jordan Justen <jordan.l.justen 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;
> > > > 
> > > > This will set it for multisampled also and upset piglit tests. We need
> > > > something of this sort:
> > > > 
> > > >             s.ClearValueAddressEnable = info->aux_usage != ISL_AUX_USAGE_MCS;
> > > 
> > > Hmmm... what piglit tests are having issues with this? And which
> > > platform?
> > 
> > On Icelake:
> > 
> > ext_framebuffer_multisample-accuracy 2 color
> > 
> > > 
> > > Also, I think it should be fine to always set it, as long as if the
> > > clear color is used (thus read from the clear color buffer), it should
> > > be correct. Maybe there's an issue with the initialization or update of
> > > the value or something like that for MCS.
> 
> And the way I read bspec, clear color address is only for CCS_D and CCS_E.
> (And for HIZ of course).

You are right, the clear color itself is only for CCS_D, CCS_E and HIZ.
But I thought it would just be ignored for other surfaces (like the
inline clear color already is ignored). However, it does have extra
notes for ICL+:

"If this bit is cleared, then no clear value is being used for the
surface. In this case, 3D Sampler will not fetch any clear value from
memory and it is assumed that the AUX_CCS auxiliary surface will never
indicate the clear state for this surface."

Maybe this means we have to explicitly disable it now?

Anyway, I'll change that locally and try it out. I think it won't hurt
for CNL, and since it fix things for ICL...

Thanks.

> > > 
> > > Why wouldn't we want to enable it for multisampled surfaces?
> > 
> > Have we enabled fast clears for MCS? I think Jason had patches but I lost
> > track.
> > 
> > > 
> > > > > +         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