[v5,06/19] intel/isl: Add support to emit clear value address.

Submitted by Rafael Antognolli on March 29, 2018, 5:58 p.m.

Details

Message ID 20180329175853.23728-7-rafael.antognolli@intel.com
State New
Headers show
Series "Use clear color address in surface state." ( rev: 5 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli March 29, 2018, 5:58 p.m.
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@intel.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
---
 src/intel/isl/isl.h               |  9 +++++++++
 src/intel/isl/isl_surface_state.c | 18 ++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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;
+         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.

Comments

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@intel.com>
> Reviewed-by: Jordan Justen <jordan.l.justen@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;

> +         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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
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@intel.com>
> > Reviewed-by: Jordan Justen <jordan.l.justen@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;

And this is actually for patch 19.
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@intel.com>
> > Reviewed-by: Jordan Justen <jordan.l.justen@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?

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.

Why wouldn't we want to enable it for multisampled surfaces?

> > +         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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
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@intel.com>
> > > Reviewed-by: Jordan Justen <jordan.l.justen@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.
> 
> 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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
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@intel.com>
> > > > Reviewed-by: Jordan Justen <jordan.l.justen@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).

> > 
> > 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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
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@intel.com>
> > > > > Reviewed-by: Jordan Justen <jordan.l.justen@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@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Tue, Apr 3, 2018 at 8:05 AM, Pohjolainen, Topi <
topi.pohjolainen@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@intel.com>
> > Reviewed-by: Jordan Justen <jordan.l.justen@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.


> > +         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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
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@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@intel.com>
> > > Reviewed-by: Jordan Justen <jordan.l.justen@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.

> 
> 
> > > +         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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
On Tue, Apr 3, 2018 at 8:23 PM, Pohjolainen, Topi <
topi.pohjolainen@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@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@intel.com>
> > > > Reviewed-by: Jordan Justen <jordan.l.justen@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?


> >
> >
> > > > +         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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
>
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@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@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@intel.com>
> > > > > Reviewed-by: Jordan Justen <jordan.l.justen@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

> 
> 
> > >
> > >
> > > > > +         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@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> >
On Tue, Apr 3, 2018 at 9:40 PM, Pohjolainen, Topi <
topi.pohjolainen@gmail.com> 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@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@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@intel.com>
> > > > > > Reviewed-by: Jordan Justen <jordan.l.justen@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
>

Ok, that's unfortunate. :-(  I guess we need to not use it for MCS which
means an assert. Unfortunately, it also means that Rafiel's plan of just
always turning it on for gen10+ is also not going to work. :-(  This is
getting unfortunately complicated.


> ext_framebuffer_multisample-accuracy 2 color
>
> >
> >
> > > >
> > > >
> > > > > > +         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@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > _______________________________________________
> > > > > mesa-dev mailing list
> > > > > mesa-dev@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > >
> > >
>
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@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@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@intel.com>
> > > > > > Reviewed-by: Jordan Justen <jordan.l.justen@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@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > _______________________________________________
> > > > > mesa-dev mailing list
> > > > > mesa-dev@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > >
> > >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu, Apr 05, 2018 at 04:11:33PM -0700, Nanley Chery wrote:
> 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@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@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@intel.com>
> > > > > > > Reviewed-by: Jordan Justen <jordan.l.justen@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.

Right you are, I keep forgetting that CCS_D/CCS_E stands for MCS when
sample count > 1. So it should work for multisampled surfaces as well. Looks
like there is more debugging for me to do to understand why the tests start
failing on ICL if clear color address is enabled for MCS surface. Thanks for
reading it more carefully than I did!

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