[v4,13/18] i965/surface_state: Emit the clear color address instead of value.

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

Details

Message ID 20180308164911.15375-14-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:49 p.m.
On Gen10, when emitting the surface state, use the value stored in the
clear color entry buffer by using a clear color address in the surface
state.

v4: Use the clear color offset from the clear_color_bo, when available.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
---
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 29 +++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
index caa92d7d878..268effef397 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -152,7 +152,7 @@  brw_emit_surface_state(struct brw_context *brw,
 
    union isl_color_value clear_color = { .u32 = { 0, 0, 0, 0 } };
 
-   struct brw_bo *aux_bo;
+   struct brw_bo *aux_bo = NULL;
    struct isl_surf *aux_surf = NULL;
    uint64_t aux_offset = 0;
    switch (aux_usage) {
@@ -186,6 +186,20 @@  brw_emit_surface_state(struct brw_context *brw,
                                  brw->isl_dev.ss.align,
                                  surf_offset);
 
+   bool use_clear_address = devinfo->gen >= 10 && aux_surf;
+
+   struct brw_bo *clear_bo = NULL;
+   uint32_t clear_offset = 0;
+   if (use_clear_address) {
+      if (mt->mcs_buf && mt->mcs_buf->clear_color_bo) {
+         clear_bo = mt->mcs_buf->clear_color_bo;
+         clear_offset = mt->mcs_buf->clear_color_offset;
+      } else {
+         clear_bo = aux_bo;
+         clear_offset = aux_offset + aux_surf->size;
+      }
+   }
+
    isl_surf_fill_state(&brw->isl_dev, state, .surf = &surf, .view = &view,
                        .address = brw_state_reloc(&brw->batch,
                                                   *surf_offset + brw->isl_dev.ss.addr_offset,
@@ -194,6 +208,8 @@  brw_emit_surface_state(struct brw_context *brw,
                        .aux_address = aux_offset,
                        .mocs = brw_get_bo_mocs(devinfo, mt->bo),
                        .clear_color = clear_color,
+                       .use_clear_address = use_clear_address,
+                       .clear_address = clear_offset,
                        .x_offset_sa = tile_x, .y_offset_sa = tile_y);
    if (aux_surf) {
       /* On gen7 and prior, the upper 20 bits of surface state DWORD 6 are the
@@ -223,6 +239,17 @@  brw_emit_surface_state(struct brw_context *brw,
 
       }
    }
+
+   if (use_clear_address) {
+      /* Make sure the offset is aligned with a cacheline. */
+      assert((clear_offset & 0x3f) == 0);
+      uint32_t *clear_address =
+            state + brw->isl_dev.ss.clear_color_state_offset;
+      *clear_address = brw_state_reloc(&brw->batch,
+                                       *surf_offset +
+                                       brw->isl_dev.ss.clear_color_state_offset,
+                                       clear_bo, *clear_address, reloc_flags);
+   }
 }
 
 static uint32_t

Comments

On Thu, Mar 08, 2018 at 08:49:06AM -0800, Rafael Antognolli wrote:
> On Gen10, when emitting the surface state, use the value stored in the
> clear color entry buffer by using a clear color address in the surface
> state.
> 
> v4: Use the clear color offset from the clear_color_bo, when available.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 29 +++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> index caa92d7d878..268effef397 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -152,7 +152,7 @@ brw_emit_surface_state(struct brw_context *brw,
>  
>     union isl_color_value clear_color = { .u32 = { 0, 0, 0, 0 } };
>  
> -   struct brw_bo *aux_bo;
> +   struct brw_bo *aux_bo = NULL;
>     struct isl_surf *aux_surf = NULL;
>     uint64_t aux_offset = 0;
>     switch (aux_usage) {
> @@ -186,6 +186,20 @@ brw_emit_surface_state(struct brw_context *brw,
>                                   brw->isl_dev.ss.align,
>                                   surf_offset);
>  
> +   bool use_clear_address = devinfo->gen >= 10 && aux_surf;
> +
> +   struct brw_bo *clear_bo = NULL;
> +   uint32_t clear_offset = 0;
> +   if (use_clear_address) {
> +      if (mt->mcs_buf && mt->mcs_buf->clear_color_bo) {
> +         clear_bo = mt->mcs_buf->clear_color_bo;
> +         clear_offset = mt->mcs_buf->clear_color_offset;

In case of "mt->mcs_buf" holding the clear value one needs to use
"mt->mcs_buf->clear_color_offset" instead of "aux_surf->size" as that
includes the space reserved for clear value and hence points past it
(aux_offset doesn't matter is it will be zero in that case):

        } else if (mt->mcs_buf) {
           clear_bo = aux_bo;
           clear_offset = mt->mcs_buf->clear_color_offset;

> +      } else {
> +         clear_bo = aux_bo;
> +         clear_offset = aux_offset + aux_surf->size;
> +      }
> +   }
> +
>     isl_surf_fill_state(&brw->isl_dev, state, .surf = &surf, .view = &view,
>                         .address = brw_state_reloc(&brw->batch,
>                                                    *surf_offset + brw->isl_dev.ss.addr_offset,
> @@ -194,6 +208,8 @@ brw_emit_surface_state(struct brw_context *brw,
>                         .aux_address = aux_offset,
>                         .mocs = brw_get_bo_mocs(devinfo, mt->bo),
>                         .clear_color = clear_color,
> +                       .use_clear_address = use_clear_address,
> +                       .clear_address = clear_offset,
>                         .x_offset_sa = tile_x, .y_offset_sa = tile_y);
>     if (aux_surf) {
>        /* On gen7 and prior, the upper 20 bits of surface state DWORD 6 are the
> @@ -223,6 +239,17 @@ brw_emit_surface_state(struct brw_context *brw,
>  
>        }
>     }
> +
> +   if (use_clear_address) {
> +      /* Make sure the offset is aligned with a cacheline. */
> +      assert((clear_offset & 0x3f) == 0);
> +      uint32_t *clear_address =
> +            state + brw->isl_dev.ss.clear_color_state_offset;
> +      *clear_address = brw_state_reloc(&brw->batch,
> +                                       *surf_offset +
> +                                       brw->isl_dev.ss.clear_color_state_offset,
> +                                       clear_bo, *clear_address, reloc_flags);
> +   }
>  }
>  
>  static uint32_t
> -- 
> 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:37 AM, Pohjolainen, Topi <
topi.pohjolainen@gmail.com> wrote:

> On Thu, Mar 08, 2018 at 08:49:06AM -0800, Rafael Antognolli wrote:
> > On Gen10, when emitting the surface state, use the value stored in the
> > clear color entry buffer by using a clear color address in the surface
> > state.
> >
> > v4: Use the clear color offset from the clear_color_bo, when available.
> >
> > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 29
> +++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > index caa92d7d878..268effef397 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -152,7 +152,7 @@ brw_emit_surface_state(struct brw_context *brw,
> >
> >     union isl_color_value clear_color = { .u32 = { 0, 0, 0, 0 } };
> >
> > -   struct brw_bo *aux_bo;
> > +   struct brw_bo *aux_bo = NULL;
> >     struct isl_surf *aux_surf = NULL;
> >     uint64_t aux_offset = 0;
> >     switch (aux_usage) {
> > @@ -186,6 +186,20 @@ brw_emit_surface_state(struct brw_context *brw,
> >                                   brw->isl_dev.ss.align,
> >                                   surf_offset);
> >
> > +   bool use_clear_address = devinfo->gen >= 10 && aux_surf;
> > +
> > +   struct brw_bo *clear_bo = NULL;
> > +   uint32_t clear_offset = 0;
> > +   if (use_clear_address) {
> > +      if (mt->mcs_buf && mt->mcs_buf->clear_color_bo) {
> > +         clear_bo = mt->mcs_buf->clear_color_bo;
> > +         clear_offset = mt->mcs_buf->clear_color_offset;
>
> In case of "mt->mcs_buf" holding the clear value one needs to use
> "mt->mcs_buf->clear_color_offset" instead of "aux_surf->size" as that
> includes the space reserved for clear value and hence points past it
> (aux_offset doesn't matter is it will be zero in that case):
>

I'm pretty sure the second case is now dead and should be deleted.


>
>         } else if (mt->mcs_buf) {
>            clear_bo = aux_bo;
>            clear_offset = mt->mcs_buf->clear_color_offset;
>
> > +      } else {
> > +         clear_bo = aux_bo;
> > +         clear_offset = aux_offset + aux_surf->size;
> > +      }
> > +   }
> > +
> >     isl_surf_fill_state(&brw->isl_dev, state, .surf = &surf, .view =
> &view,
> >                         .address = brw_state_reloc(&brw->batch,
> >                                                    *surf_offset +
> brw->isl_dev.ss.addr_offset,
> > @@ -194,6 +208,8 @@ brw_emit_surface_state(struct brw_context *brw,
> >                         .aux_address = aux_offset,
> >                         .mocs = brw_get_bo_mocs(devinfo, mt->bo),
> >                         .clear_color = clear_color,
> > +                       .use_clear_address = use_clear_address,
> > +                       .clear_address = clear_offset,
> >                         .x_offset_sa = tile_x, .y_offset_sa = tile_y);
> >     if (aux_surf) {
> >        /* On gen7 and prior, the upper 20 bits of surface state DWORD 6
> are the
> > @@ -223,6 +239,17 @@ brw_emit_surface_state(struct brw_context *brw,
> >
> >        }
> >     }
> > +
> > +   if (use_clear_address) {
> > +      /* Make sure the offset is aligned with a cacheline. */
> > +      assert((clear_offset & 0x3f) == 0);
> > +      uint32_t *clear_address =
> > +            state + brw->isl_dev.ss.clear_color_state_offset;
> > +      *clear_address = brw_state_reloc(&brw->batch,
> > +                                       *surf_offset +
> > +                                       brw->isl_dev.ss.clear_color_
> state_offset,
> > +                                       clear_bo, *clear_address,
> reloc_flags);
> > +   }
> >  }
> >
> >  static uint32_t
> > --
> > 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:19:43AM -0700, Jason Ekstrand wrote:
> On Tue, Mar 27, 2018 at 4:37 AM, Pohjolainen, Topi <topi.pohjolainen@gmail.com>
> wrote:
> 
>     On Thu, Mar 08, 2018 at 08:49:06AM -0800, Rafael Antognolli wrote:
>     > On Gen10, when emitting the surface state, use the value stored in the
>     > clear color entry buffer by using a clear color address in the surface
>     > state.
>     >
>     > v4: Use the clear color offset from the clear_color_bo, when available.
>     >
>     > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
>     > ---
>     >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 29
>     +++++++++++++++++++++++-
>     >  1 file changed, 28 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/
>     drivers/dri/i965/brw_wm_surface_state.c
>     > index caa92d7d878..268effef397 100644
>     > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>     > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>     > @@ -152,7 +152,7 @@ brw_emit_surface_state(struct brw_context *brw,
>     >
>     >     union isl_color_value clear_color = { .u32 = { 0, 0, 0, 0 } };
>     >
>     > -   struct brw_bo *aux_bo;
>     > +   struct brw_bo *aux_bo = NULL;
>     >     struct isl_surf *aux_surf = NULL;
>     >     uint64_t aux_offset = 0;
>     >     switch (aux_usage) {
>     > @@ -186,6 +186,20 @@ brw_emit_surface_state(struct brw_context *brw,
>     >                                   brw->isl_dev.ss.align,
>     >                                   surf_offset);
>     >
>     > +   bool use_clear_address = devinfo->gen >= 10 && aux_surf;
>     > +
>     > +   struct brw_bo *clear_bo = NULL;
>     > +   uint32_t clear_offset = 0;
>     > +   if (use_clear_address) {
>     > +      if (mt->mcs_buf && mt->mcs_buf->clear_color_bo) {
>     > +         clear_bo = mt->mcs_buf->clear_color_bo;
>     > +         clear_offset = mt->mcs_buf->clear_color_offset;
> 
>     In case of "mt->mcs_buf" holding the clear value one needs to use
>     "mt->mcs_buf->clear_color_offset" instead of "aux_surf->size" as that
>     includes the space reserved for clear value and hence points past it
>     (aux_offset doesn't matter is it will be zero in that case):
> 
> 
> I'm pretty sure the second case is now dead and should be deleted.

Ugh, that's right, I removed it from brw_blorp.c but forgot it here :-/

>             } else if (mt->mcs_buf) {
>                clear_bo = aux_bo;
>                clear_offset = mt->mcs_buf->clear_color_offset;
> 
>     > +      } else {
>     > +         clear_bo = aux_bo;
>     > +         clear_offset = aux_offset + aux_surf->size;
>     > +      }
>     > +   }
>     > +
>     >     isl_surf_fill_state(&brw->isl_dev, state, .surf = &surf, .view = &
>     view,
>     >                         .address = brw_state_reloc(&brw->batch,
>     >                                                    *surf_offset + brw->
>     isl_dev.ss.addr_offset,
>     > @@ -194,6 +208,8 @@ brw_emit_surface_state(struct brw_context *brw,
>     >                         .aux_address = aux_offset,
>     >                         .mocs = brw_get_bo_mocs(devinfo, mt->bo),
>     >                         .clear_color = clear_color,
>     > +                       .use_clear_address = use_clear_address,
>     > +                       .clear_address = clear_offset,
>     >                         .x_offset_sa = tile_x, .y_offset_sa = tile_y);
>     >     if (aux_surf) {
>     >        /* On gen7 and prior, the upper 20 bits of surface state DWORD 6
>     are the
>     > @@ -223,6 +239,17 @@ brw_emit_surface_state(struct brw_context *brw,
>     >
>     >        }
>     >     }
>     > +
>     > +   if (use_clear_address) {
>     > +      /* Make sure the offset is aligned with a cacheline. */
>     > +      assert((clear_offset & 0x3f) == 0);
>     > +      uint32_t *clear_address =
>     > +            state + brw->isl_dev.ss.clear_color_state_offset;
>     > +      *clear_address = brw_state_reloc(&brw->batch,
>     > +                                       *surf_offset +
>     > +                                       brw->isl_dev.ss.clear_color_
>     state_offset,
>     > +                                       clear_bo, *clear_address,
>     reloc_flags);
>     > +   }
>     >  }
>     >
>     >  static uint32_t
>     > --
>     > 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
> 
>