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

[Nanley Chery]

On Wed, Apr 04, 2018 at 07:40:43AM +0300, Pohjolainen, Topi wrote:
> On Tue, Apr 03, 2018 at 09:38:52PM -0700, Jason Ekstrand wrote:
> > On Tue, Apr 3, 2018 at 8:23 PM, Pohjolainen, Topi <
> > topi.pohjolainen at gmail.com> wrote:
> > 
> > > On Tue, Apr 03, 2018 at 02:55:31PM -0700, Jason Ekstrand wrote:
> > > > On Tue, Apr 3, 2018 at 8:05 AM, Pohjolainen, Topi <
> > > > topi.pohjolainen at gmail.com> 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;
> > > > >
> > > >
> > > > Can we assert instead?  If the caller asks for a clear address to be set
> > > > they should get it and not have it magically disabled when they ask for
> > > MCS.
> > >
> > > Right, here an assert would be just fine. I should have made the comment
> > > in patch in 19 where it belongs. There we start setting the flag
> > > unconditionally for "info->aux_usage != ISL_AUX_USAGE_NONE". If we don't,
> > > then on ICL we start failing multisampling tests.
> > >
> > 
> > Is this a known hardware restriction or is there just something weird going
> > on with MCS?
> 
> Bspec says the clear address is only for CCS_D and CCS_E. And setting it for
> MCS seems to upset both the simulator and hardware. At least with
> 
> ext_framebuffer_multisample-accuracy 2 color
> 

Could you help me find the Bspec restriction?

The Bspec says that the clear address exists for

[Auxiliary Surface Mode] == 'AUX_CCS_D' OR
[Auxiliary Surface Mode] == 'AUX_CCS_E'

In the definition of Auxiliary Surface Mode, it states that AUX_CCS_* is
equal to MCS if the sample count > 1.

-Nanley

> > 
> > 
> > > >
> > > >
> > > > > > +         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
> > > > >
> > >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev