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

Submitted by Rafael Antognolli on Feb. 21, 2018, 9:45 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Rafael Antognolli Feb. 21, 2018, 9:45 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 f1b38efed44..bab0ad3d544 100644
--- a/src/intel/isl/isl.h
+++ b/src/intel/isl/isl.h
@@ -1306,6 +1306,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 bfb27fa4a44..e7c489df912 100644
--- a/src/intel/isl/isl_surface_state.c
+++ b/src/intel/isl/isl_surface_state.c
@@ -635,11 +635,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 2018-02-21 13:45:15, 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 f1b38efed44..bab0ad3d544 100644
> --- a/src/intel/isl/isl.h
> +++ b/src/intel/isl/isl.h
> @@ -1306,6 +1306,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;

I'm still wondering about this field. I think at the end we can just a
assume that if gen >= 10 and aux_usage != ISL_AUX_USAGE_NONE, then
we'll use the address.

I think you mentioned that it could be tough implement the support in
steps if we had an all or nothing enaling of the address usage. But,
does that mean that at the end of your series you could add a patch to
remove this `use_clear_address` field?

Maybe as a test in jenkins, you could add a patch that asserts that if
gen >= 10 and there is an aux_buffer, then use_clear_address==true in
your current series.

-Jordan

> +   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 bfb27fa4a44..e7c489df912 100644
> --- a/src/intel/isl/isl_surface_state.c
> +++ b/src/intel/isl/isl_surface_state.c
> @@ -635,11 +635,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.
> -- 
> 2.14.3
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Mon, Feb 26, 2018 at 1:14 PM, Jordan Justen <jordan.l.justen@intel.com>
wrote:

> On 2018-02-21 13:45:15, 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 f1b38efed44..bab0ad3d544 100644
> > --- a/src/intel/isl/isl.h
> > +++ b/src/intel/isl/isl.h
> > @@ -1306,6 +1306,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.
>

I would recommend a slightly different comment:

/**
If set, the Clear Value Address field is set to point to a CLEAR_COLOR
state and the inline clear color fields are left empty.
*/



> > +    */
> > +   bool use_clear_address;
>
> I'm still wondering about this field. I think at the end we can just a
> assume that if gen >= 10 and aux_usage != ISL_AUX_USAGE_NONE, then
> we'll use the address.
>

That's not going to work if we want to turn this on for blorp, anv, and
i965 separately.


> I think you mentioned that it could be tough implement the support in
> steps if we had an all or nothing enaling of the address usage. But,
> does that mean that at the end of your series you could add a patch to
> remove this `use_clear_address` field?
>
> Maybe as a test in jenkins, you could add a patch that asserts that if
> gen >= 10 and there is an aux_buffer, then use_clear_address==true in
> your current series.
>
> -Jordan
>
> > +   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 bfb27fa4a44..e7c489df912 100644
> > --- a/src/intel/isl/isl_surface_state.c
> > +++ b/src/intel/isl/isl_surface_state.c
> > @@ -635,11 +635,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.
> > --
> > 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 2018-02-26 17:08:12, Jason Ekstrand wrote:
> On Mon, Feb 26, 2018 at 1:14 PM, Jordan Justen <jordan.l.justen@intel.com>
> wrote:
> 
> > On 2018-02-21 13:45:15, Rafael Antognolli wrote:
> > > +   bool use_clear_address;
> >
> > I'm still wondering about this field. I think at the end we can just a
> > assume that if gen >= 10 and aux_usage != ISL_AUX_USAGE_NONE, then
> > we'll use the address.
> >
> 
> That's not going to work if we want to turn this on for blorp, anv, and
> i965 separately.

I guess this goes to the point I mentioned below. Maybe it make it
easier to break it up for enabling it. (I'm not certain that we
couldn't slice it up another way, but the argument seems fine.)

But, after that, is it needed? If it's alway enabled when gen >= 10
and aux_usage != ISL_AUX_USAGE_NONE, then once everything is in place,
then isl can easily check for that condition, and there's no purpose
for use_clear_address. Correct?

I also wonder if clear_address is needed in the info struct. It did
not look like blorp set it by the end of the series, yet blorp was
enabling the feature. (I'm guessing that the reloc must be handing the
aux buf offset for blorp.)

-Jordan

> > I think you mentioned that it could be tough implement the support in
> > steps if we had an all or nothing enaling of the address usage. But,
> > does that mean that at the end of your series you could add a patch to
> > remove this `use_clear_address` field?
> >
> > Maybe as a test in jenkins, you could add a patch that asserts that if
> > gen >= 10 and there is an aux_buffer, then use_clear_address==true in
> > your current series.
> >
> > -Jordan
On Mon, Feb 26, 2018 at 8:12 PM, Jordan Justen <jordan.l.justen@intel.com>
wrote:

> On 2018-02-26 17:08:12, Jason Ekstrand wrote:
> > On Mon, Feb 26, 2018 at 1:14 PM, Jordan Justen <
> jordan.l.justen@intel.com>
> > wrote:
> >
> > > On 2018-02-21 13:45:15, Rafael Antognolli wrote:
> > > > +   bool use_clear_address;
> > >
> > > I'm still wondering about this field. I think at the end we can just a
> > > assume that if gen >= 10 and aux_usage != ISL_AUX_USAGE_NONE, then
> > > we'll use the address.
> > >
> >
> > That's not going to work if we want to turn this on for blorp, anv, and
> > i965 separately.
>
> I guess this goes to the point I mentioned below. Maybe it make it
> easier to break it up for enabling it. (I'm not certain that we
> couldn't slice it up another way, but the argument seems fine.)
>
> But, after that, is it needed? If it's alway enabled when gen >= 10
> and aux_usage != ISL_AUX_USAGE_NONE, then once everything is in place,
> then isl can easily check for that condition, and there's no purpose
> for use_clear_address. Correct?
>

I suppose.  Once everything's moved over, there's really no reason to keep
it around on gen10.


> I also wonder if clear_address is needed in the info struct. It did
> not look like blorp set it by the end of the series, yet blorp was
> enabling the feature. (I'm guessing that the reloc must be handing the
> aux buf offset for blorp.)
>

Yes and no.  It's not really used today but it is needed the moment we get
rid of relocations.

--Jason



> -Jordan
>
> > > I think you mentioned that it could be tough implement the support in
> > > steps if we had an all or nothing enaling of the address usage. But,
> > > does that mean that at the end of your series you could add a patch to
> > > remove this `use_clear_address` field?
> > >
> > > Maybe as a test in jenkins, you could add a patch that asserts that if
> > > gen >= 10 and there is an aux_buffer, then use_clear_address==true in
> > > your current series.
> > >
> > > -Jordan
>
On 2018-02-26 21:35:42, Jason Ekstrand wrote:
> On Mon, Feb 26, 2018 at 8:12 PM, Jordan Justen <jordan.l.justen@intel.com>
> wrote:
> 
> > On 2018-02-26 17:08:12, Jason Ekstrand wrote:
> > > On Mon, Feb 26, 2018 at 1:14 PM, Jordan Justen <
> > jordan.l.justen@intel.com>
> > > wrote:
> > >
> > > > On 2018-02-21 13:45:15, Rafael Antognolli wrote:
> > > > > +   bool use_clear_address;
> > > >
> > > > I'm still wondering about this field. I think at the end we can just a
> > > > assume that if gen >= 10 and aux_usage != ISL_AUX_USAGE_NONE, then
> > > > we'll use the address.
> > > >
> > >
> > > That's not going to work if we want to turn this on for blorp, anv, and
> > > i965 separately.
> >
> > I guess this goes to the point I mentioned below. Maybe it make it
> > easier to break it up for enabling it. (I'm not certain that we
> > couldn't slice it up another way, but the argument seems fine.)
> >
> > But, after that, is it needed? If it's alway enabled when gen >= 10
> > and aux_usage != ISL_AUX_USAGE_NONE, then once everything is in place,
> > then isl can easily check for that condition, and there's no purpose
> > for use_clear_address. Correct?
> >
> 
> I suppose.  Once everything's moved over, there's really no reason to keep
> it around on gen10.
> 
> > I also wonder if clear_address is needed in the info struct. It did
> > not look like blorp set it by the end of the series, yet blorp was
> > enabling the feature. (I'm guessing that the reloc must be handing the
> > aux buf offset for blorp.)
> >
> 
> Yes and no.  It's not really used today but it is needed the moment we get
> rid of relocations.

Ah. Good point.

I guess the pinned address could still be written by the 'reloc'
function, without emitting the reloc. That would also mean
clear_address wouldn't be needed in the isl info struct. The name
'reloc' starts adding confusion in this case, and maybe something like
emit_bo_address might make more sense at that point.

Or, we could make the reloc functions total no-ops with pinned
addresses, in which case clear_address would be needed.

Based on this, we might want to decide if patch 7 should be doing
anything with clear_address... (Right now it doesn't set it.)

-Jordan

> >
> > > > I think you mentioned that it could be tough implement the support in
> > > > steps if we had an all or nothing enaling of the address usage. But,
> > > > does that mean that at the end of your series you could add a patch to
> > > > remove this `use_clear_address` field?
> > > >
> > > > Maybe as a test in jenkins, you could add a patch that asserts that if
> > > > gen >= 10 and there is an aux_buffer, then use_clear_address==true in
> > > > your current series.
> > > >
> > > > -Jordan
> >
On Tue, Feb 27, 2018 at 05:00:24PM -0800, Jordan Justen wrote:
> On 2018-02-26 21:35:42, Jason Ekstrand wrote:
> > On Mon, Feb 26, 2018 at 8:12 PM, Jordan Justen <jordan.l.justen@intel.com>
> > wrote:
> > 
> > > On 2018-02-26 17:08:12, Jason Ekstrand wrote:
> > > > On Mon, Feb 26, 2018 at 1:14 PM, Jordan Justen <
> > > jordan.l.justen@intel.com>
> > > > wrote:
> > > >
> > > > > On 2018-02-21 13:45:15, Rafael Antognolli wrote:
> > > > > > +   bool use_clear_address;
> > > > >
> > > > > I'm still wondering about this field. I think at the end we can just a
> > > > > assume that if gen >= 10 and aux_usage != ISL_AUX_USAGE_NONE, then
> > > > > we'll use the address.
> > > > >
> > > >
> > > > That's not going to work if we want to turn this on for blorp, anv, and
> > > > i965 separately.
> > >
> > > I guess this goes to the point I mentioned below. Maybe it make it
> > > easier to break it up for enabling it. (I'm not certain that we
> > > couldn't slice it up another way, but the argument seems fine.)
> > >
> > > But, after that, is it needed? If it's alway enabled when gen >= 10
> > > and aux_usage != ISL_AUX_USAGE_NONE, then once everything is in place,
> > > then isl can easily check for that condition, and there's no purpose
> > > for use_clear_address. Correct?
> > >
> > 
> > I suppose.  Once everything's moved over, there's really no reason to keep
> > it around on gen10.
> > 
> > > I also wonder if clear_address is needed in the info struct. It did
> > > not look like blorp set it by the end of the series, yet blorp was
> > > enabling the feature. (I'm guessing that the reloc must be handing the
> > > aux buf offset for blorp.)
> > >
> > 
> > Yes and no.  It's not really used today but it is needed the moment we get
> > rid of relocations.
> 
> Ah. Good point.
> 
> I guess the pinned address could still be written by the 'reloc'
> function, without emitting the reloc. That would also mean
> clear_address wouldn't be needed in the isl info struct. The name
> 'reloc' starts adding confusion in this case, and maybe something like
> emit_bo_address might make more sense at that point.
> 
> Or, we could make the reloc functions total no-ops with pinned
> addresses, in which case clear_address would be needed.
> 
> Based on this, we might want to decide if patch 7 should be doing
> anything with clear_address... (Right now it doesn't set it.)

If that's the case, then we not only need to set clear_address, but also
address and aux_address (they are also not being set there).

> > >
> > > > > I think you mentioned that it could be tough implement the support in
> > > > > steps if we had an all or nothing enaling of the address usage. But,
> > > > > does that mean that at the end of your series you could add a patch to
> > > > > remove this `use_clear_address` field?
> > > > >
> > > > > Maybe as a test in jenkins, you could add a patch that asserts that if
> > > > > gen >= 10 and there is an aux_buffer, then use_clear_address==true in
> > > > > your current series.
> > > > >
> > > > > -Jordan
> > >