Re: [Mesa-dev] [PATCH v3 5/9] i965/wm_surface_state: Use the clear address if it's non-zero

[Jason Ekstrand]

Sounds good


On April 23, 2018 18:07:19 Nanley Chery  wrote:


On Mon, Apr 23, 2018 at 11:18:54AM -0700, Jason Ekstrand wrote:
> I think you want to say "clear_color_bo is non-NULL" in the commit message
> rather than talking about addresses.  Otherwise, this looks like a very
> nice clean-up.
>

To make sure we're on the same page, I made an error in using the term
clear address, because the code only checks part of it (the bo)?

What do you think of this new commit message?

   i965/wm_surface_state: Use the clear address if clear_bo is non-NULL

   We want to add and use a getter that turns off the indirect path by
   returning zero for the clear color bo and offset.

   v2: Fix usage of "clear address" in commit message (Jason).

> Reviewed-by: Jason Ekstrand 
>

Thanks!

-Nanley

> On Wed, Apr 11, 2018 at 1:42 PM, Nanley Chery  wrote:
>
> > We want to add and use a getter that turns off the indirect path by
> > returning zero for the clear address.
> > ---
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 17 ++---
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > 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 06f739faf61..3c70dbcc110 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -155,6 +155,8 @@ brw_emit_surface_state(struct brw_context *brw,
> > struct brw_bo *aux_bo = NULL;
> > struct isl_surf *aux_surf = NULL;
> > uint64_t aux_offset = 0;
> > +   struct brw_bo *clear_bo = NULL;
> > +   uint32_t clear_offset = 0;
> > struct intel_miptree_aux_buffer *aux_buf =
> > intel_miptree_get_aux_buffer(mt);
> >
> > if (aux_usage != ISL_AUX_USAGE_NONE) {
> > @@ -165,6 +167,8 @@ brw_emit_surface_state(struct brw_context *brw,
> >/* We only really need a clear color if we also have an auxiliary
> > * surface.  Without one, it does nothing.
> > */
> > +  clear_bo = aux_buf->clear_color_bo;
> > +  clear_offset = aux_buf->clear_color_offset;
> >clear_color = mt->fast_clear_color;
> > }
> >
> > @@ -173,15 +177,6 @@ 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) {
> > -  clear_bo = aux_buf->clear_color_bo;
> > -  clear_offset = aux_buf->clear_color_offset;
> > -   }
> > -
> > isl_surf_fill_state(>isl_dev, state, .surf = , .view =
> > ,
> > .address = brw_state_reloc(>batch,
> >*surf_offset +
> > brw->isl_dev.ss.addr_offset,
> > @@ -190,7 +185,7 @@ 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,
> > +   .use_clear_address = clear_bo != NULL,
> > .clear_address = clear_offset,
> > .x_offset_sa = tile_x, .y_offset_sa = tile_y);
> > if (aux_surf) {
> > @@ -222,7 +217,7 @@ brw_emit_surface_state(struct brw_context *brw,
> >}
> > }
> >
> > -   if (use_clear_address) {
> > +   if (clear_bo != NULL) {
> >/* Make sure the offset is aligned with a cacheline. */
> >assert((clear_offset & 0x3f) == 0);
> >uint32_t *clear_address =
> > --
> > 2.16.2
> >
> > ___
> > 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

Re: [Mesa-dev] [PATCH v3 5/9] i965/wm_surface_state: Use the clear address if it's non-zero

[Nanley Chery]

On Mon, Apr 23, 2018 at 11:18:54AM -0700, Jason Ekstrand wrote:
> I think you want to say "clear_color_bo is non-NULL" in the commit message
> rather than talking about addresses.  Otherwise, this looks like a very
> nice clean-up.
> 

To make sure we're on the same page, I made an error in using the term
clear address, because the code only checks part of it (the bo)?

What do you think of this new commit message?

i965/wm_surface_state: Use the clear address if clear_bo is non-NULL

We want to add and use a getter that turns off the indirect path by
returning zero for the clear color bo and offset.

v2: Fix usage of "clear address" in commit message (Jason).

> Reviewed-by: Jason Ekstrand 
> 

Thanks!

-Nanley

> On Wed, Apr 11, 2018 at 1:42 PM, Nanley Chery  wrote:
> 
> > We want to add and use a getter that turns off the indirect path by
> > returning zero for the clear address.
> > ---
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 17 ++---
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > 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 06f739faf61..3c70dbcc110 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -155,6 +155,8 @@ brw_emit_surface_state(struct brw_context *brw,
> > struct brw_bo *aux_bo = NULL;
> > struct isl_surf *aux_surf = NULL;
> > uint64_t aux_offset = 0;
> > +   struct brw_bo *clear_bo = NULL;
> > +   uint32_t clear_offset = 0;
> > struct intel_miptree_aux_buffer *aux_buf =
> > intel_miptree_get_aux_buffer(mt);
> >
> > if (aux_usage != ISL_AUX_USAGE_NONE) {
> > @@ -165,6 +167,8 @@ brw_emit_surface_state(struct brw_context *brw,
> >/* We only really need a clear color if we also have an auxiliary
> > * surface.  Without one, it does nothing.
> > */
> > +  clear_bo = aux_buf->clear_color_bo;
> > +  clear_offset = aux_buf->clear_color_offset;
> >clear_color = mt->fast_clear_color;
> > }
> >
> > @@ -173,15 +177,6 @@ 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) {
> > -  clear_bo = aux_buf->clear_color_bo;
> > -  clear_offset = aux_buf->clear_color_offset;
> > -   }
> > -
> > isl_surf_fill_state(>isl_dev, state, .surf = , .view =
> > ,
> > .address = brw_state_reloc(>batch,
> >*surf_offset +
> > brw->isl_dev.ss.addr_offset,
> > @@ -190,7 +185,7 @@ 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,
> > +   .use_clear_address = clear_bo != NULL,
> > .clear_address = clear_offset,
> > .x_offset_sa = tile_x, .y_offset_sa = tile_y);
> > if (aux_surf) {
> > @@ -222,7 +217,7 @@ brw_emit_surface_state(struct brw_context *brw,
> >}
> > }
> >
> > -   if (use_clear_address) {
> > +   if (clear_bo != NULL) {
> >/* Make sure the offset is aligned with a cacheline. */
> >assert((clear_offset & 0x3f) == 0);
> >uint32_t *clear_address =
> > --
> > 2.16.2
> >
> > ___
> > 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

Re: [Mesa-dev] [PATCH v3 5/9] i965/wm_surface_state: Use the clear address if it's non-zero

[Jason Ekstrand]

I think you want to say "clear_color_bo is non-NULL" in the commit message
rather than talking about addresses.  Otherwise, this looks like a very
nice clean-up.

Reviewed-by: Jason Ekstrand 

On Wed, Apr 11, 2018 at 1:42 PM, Nanley Chery  wrote:

> We want to add and use a getter that turns off the indirect path by
> returning zero for the clear address.
> ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> 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 06f739faf61..3c70dbcc110 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -155,6 +155,8 @@ brw_emit_surface_state(struct brw_context *brw,
> struct brw_bo *aux_bo = NULL;
> struct isl_surf *aux_surf = NULL;
> uint64_t aux_offset = 0;
> +   struct brw_bo *clear_bo = NULL;
> +   uint32_t clear_offset = 0;
> struct intel_miptree_aux_buffer *aux_buf =
> intel_miptree_get_aux_buffer(mt);
>
> if (aux_usage != ISL_AUX_USAGE_NONE) {
> @@ -165,6 +167,8 @@ brw_emit_surface_state(struct brw_context *brw,
>/* We only really need a clear color if we also have an auxiliary
> * surface.  Without one, it does nothing.
> */
> +  clear_bo = aux_buf->clear_color_bo;
> +  clear_offset = aux_buf->clear_color_offset;
>clear_color = mt->fast_clear_color;
> }
>
> @@ -173,15 +177,6 @@ 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) {
> -  clear_bo = aux_buf->clear_color_bo;
> -  clear_offset = aux_buf->clear_color_offset;
> -   }
> -
> isl_surf_fill_state(>isl_dev, state, .surf = , .view =
> ,
> .address = brw_state_reloc(>batch,
>*surf_offset +
> brw->isl_dev.ss.addr_offset,
> @@ -190,7 +185,7 @@ 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,
> +   .use_clear_address = clear_bo != NULL,
> .clear_address = clear_offset,
> .x_offset_sa = tile_x, .y_offset_sa = tile_y);
> if (aux_surf) {
> @@ -222,7 +217,7 @@ brw_emit_surface_state(struct brw_context *brw,
>}
> }
>
> -   if (use_clear_address) {
> +   if (clear_bo != NULL) {
>/* Make sure the offset is aligned with a cacheline. */
>assert((clear_offset & 0x3f) == 0);
>uint32_t *clear_address =
> --
> 2.16.2
>
> ___
> 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

Re: [Mesa-dev] [PATCH v3 5/9] i965/wm_surface_state: Use the clear address if it's non-zero

[Rafael Antognolli]

Looks like this is the last patch in the series that is only cleaning up
the mess I left behind by adding the clear color bo. Thanks for that.

Reviewed-by: Rafael Antognolli 

On Wed, Apr 11, 2018 at 01:42:22PM -0700, Nanley Chery wrote:
> We want to add and use a getter that turns off the indirect path by
> returning zero for the clear address.
> ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> 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 06f739faf61..3c70dbcc110 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -155,6 +155,8 @@ brw_emit_surface_state(struct brw_context *brw,
> struct brw_bo *aux_bo = NULL;
> struct isl_surf *aux_surf = NULL;
> uint64_t aux_offset = 0;
> +   struct brw_bo *clear_bo = NULL;
> +   uint32_t clear_offset = 0;
> struct intel_miptree_aux_buffer *aux_buf = 
> intel_miptree_get_aux_buffer(mt);
>  
> if (aux_usage != ISL_AUX_USAGE_NONE) {
> @@ -165,6 +167,8 @@ brw_emit_surface_state(struct brw_context *brw,
>/* We only really need a clear color if we also have an auxiliary
> * surface.  Without one, it does nothing.
> */
> +  clear_bo = aux_buf->clear_color_bo;
> +  clear_offset = aux_buf->clear_color_offset;
>clear_color = mt->fast_clear_color;
> }
>  
> @@ -173,15 +177,6 @@ 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) {
> -  clear_bo = aux_buf->clear_color_bo;
> -  clear_offset = aux_buf->clear_color_offset;
> -   }
> -
> isl_surf_fill_state(>isl_dev, state, .surf = , .view = ,
> .address = brw_state_reloc(>batch,
>*surf_offset + 
> brw->isl_dev.ss.addr_offset,
> @@ -190,7 +185,7 @@ 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,
> +   .use_clear_address = clear_bo != NULL,
> .clear_address = clear_offset,
> .x_offset_sa = tile_x, .y_offset_sa = tile_y);
> if (aux_surf) {
> @@ -222,7 +217,7 @@ brw_emit_surface_state(struct brw_context *brw,
>}
> }
>  
> -   if (use_clear_address) {
> +   if (clear_bo != NULL) {
>/* Make sure the offset is aligned with a cacheline. */
>assert((clear_offset & 0x3f) == 0);
>uint32_t *clear_address =
> -- 
> 2.16.2
> 
> ___
> 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] [PATCH v3 5/9] i965/wm_surface_state: Use the clear address if it's non-zero

[Nanley Chery]

We want to add and use a getter that turns off the indirect path by
returning zero for the clear address.
---
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

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 06f739faf61..3c70dbcc110 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -155,6 +155,8 @@ brw_emit_surface_state(struct brw_context *brw,
struct brw_bo *aux_bo = NULL;
struct isl_surf *aux_surf = NULL;
uint64_t aux_offset = 0;
+   struct brw_bo *clear_bo = NULL;
+   uint32_t clear_offset = 0;
struct intel_miptree_aux_buffer *aux_buf = intel_miptree_get_aux_buffer(mt);
 
if (aux_usage != ISL_AUX_USAGE_NONE) {
@@ -165,6 +167,8 @@ brw_emit_surface_state(struct brw_context *brw,
   /* We only really need a clear color if we also have an auxiliary
* surface.  Without one, it does nothing.
*/
+  clear_bo = aux_buf->clear_color_bo;
+  clear_offset = aux_buf->clear_color_offset;
   clear_color = mt->fast_clear_color;
}
 
@@ -173,15 +177,6 @@ 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) {
-  clear_bo = aux_buf->clear_color_bo;
-  clear_offset = aux_buf->clear_color_offset;
-   }
-
isl_surf_fill_state(>isl_dev, state, .surf = , .view = ,
.address = brw_state_reloc(>batch,
   *surf_offset + 
brw->isl_dev.ss.addr_offset,
@@ -190,7 +185,7 @@ 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,
+   .use_clear_address = clear_bo != NULL,
.clear_address = clear_offset,
.x_offset_sa = tile_x, .y_offset_sa = tile_y);
if (aux_surf) {
@@ -222,7 +217,7 @@ brw_emit_surface_state(struct brw_context *brw,
   }
}
 
-   if (use_clear_address) {
+   if (clear_bo != NULL) {
   /* Make sure the offset is aligned with a cacheline. */
   assert((clear_offset & 0x3f) == 0);
   uint32_t *clear_address =
-- 
2.16.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev