[v4,05/18] intel/isl: Add support to emit clear value address.

Submitted by Rafael Antognolli on March 8, 2018, 4:48 p.m.

Details

Message ID 20180308164911.15375-6-rafael.antognolli@intel.com
State New
Headers show
Series "Use clear color address in surface state." ( rev: 4 3 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli March 8, 2018, 4:48 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>
---
 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 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@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;
#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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Tue, Mar 27, 2018 at 4:31 AM, Pohjolainen, Topi <
topi.pohjolainen@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@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.


> #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@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, Mar 27, 2018 at 11:01:34AM -0700, Jason Ekstrand wrote:
> On Tue, Mar 27, 2018 at 4:31 AM, Pohjolainen, Topi <
> topi.pohjolainen@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@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@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
> >