Re: [Mesa-dev] [PATCH v13 21/36] i965: Restructure CCS disabling

[Jason Ekstrand]

On Fri, May 19, 2017 at 2:38 AM, Daniel Stone  wrote:

> From: Ben Widawsky 
>
> Make the code only disable CCS when it has to, unlike before where it
> disabled CCS and enabled it when it could. This is much more inline with
> how it should work in a few patches, where we have fewer restrictions as
> to when we disable CCS.
>
> v2: Change CCS disabling to an assertion in layout creation (Topi)
>
> v3: Make sure to disable aux buffers when creating a miptree from a BO.
> Today, this only happens via intel_update_image_buffer. At the end of
> the modifier series, we should be able to undo this. Some fixes from
> Topi in here as well.
>
> Cc: "Pohjolainen, Topi" 
> Signed-off-by: Ben Widawsky 
>
> Topi's changes
>
> Signed-off-by: Daniel Stone 
> ---
>  src/mesa/drivers/dri/i965/brw_context.c   |  3 +++
>  src/mesa/drivers/dri/i965/intel_fbo.h |  7 +++
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 22 +-
>  3 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> index 5055dd76a8..1a2b64f73e 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -1718,6 +1718,9 @@ intel_update_image_buffer(struct brw_context *intel,
> if (last_mt && last_mt->bo == buffer->bo)
>return;
>
> +   if (!buffer->aux_offset)
> +  rb->no_aux = true;
> +
> intel_update_winsys_renderbuffer_miptree(intel, rb, buffer->bo,
>  buffer->width, buffer->height,
>  buffer->pitch);
> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.h
> b/src/mesa/drivers/dri/i965/intel_fbo.h
> index 08b82e8934..9265aab2e2 100644
> --- a/src/mesa/drivers/dri/i965/intel_fbo.h
> +++ b/src/mesa/drivers/dri/i965/intel_fbo.h
> @@ -111,6 +111,13 @@ struct intel_renderbuffer
>  * for the duration of a mapping.
>  */
> bool singlesample_mt_is_tmp;
> +
> +   /**
> +* Set to true if this buffer definitely does not have auxiliary data,
> like
> +* CCS, associated with it. It's generally to be used when importing a
> +* DRIimage, where that DRIimage had no modifier.
> +*/
> +   bool no_aux;
>

I think I would mildly prefer the stuff having to do with this bool to be
its own patch right after this one.


>  };
>
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index a8564d9573..9dca5cc435 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -328,7 +328,6 @@ intel_miptree_create_layout(struct brw_context *brw,
> mt->logical_depth0 = depth0;
> mt->aux_disable = (layout_flags & MIPTREE_LAYOUT_DISABLE_AUX) != 0 ?
>INTEL_AUX_DISABLE_ALL : INTEL_AUX_DISABLE_NONE;
> -   mt->aux_disable |= INTEL_AUX_DISABLE_CCS;
> mt->is_scanout = (layout_flags & MIPTREE_LAYOUT_FOR_SCANOUT) != 0;
> exec_list_make_empty(>hiz_map);
> exec_list_make_empty(>color_resolve_map);
> @@ -521,6 +520,8 @@ intel_miptree_create_layout(struct brw_context *brw,
> } else if (brw->gen >= 9 && num_samples > 1) {
>layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
> } else {
> +  mt->aux_disable |= INTEL_AUX_DISABLE_CCS;
> +
>const UNUSED bool is_lossless_compressed_aux =
>   brw->gen >= 9 && num_samples == 1 &&
>   mt->format == MESA_FORMAT_R_UINT32;
> @@ -696,7 +697,6 @@ intel_miptree_create(struct brw_context *brw,
>  */
> if (intel_tiling_supports_non_msrt_mcs(brw, mt->tiling) &&
> intel_miptree_supports_non_msrt_fast_clear(brw, mt)) {
> -  mt->aux_disable &= ~INTEL_AUX_DISABLE_CCS;
>assert(brw->gen < 8 || mt->halign == 16 || num_samples <= 1);
>
>/* On Gen9+ clients are not currently capable of consuming
> compressed
> @@ -710,8 +710,11 @@ intel_miptree_create(struct brw_context *brw,
>   intel_miptree_supports_lossless_compressed(brw, mt);
>
>if (is_lossless_compressed) {
> + assert((mt->aux_disable & INTEL_AUX_DISABLE_CCS) == 0);
>

Mind making this assert(!(thing)) rather than assert((thing) == 0)?  That
would match the one a few lines below.


>   intel_miptree_alloc_non_msrt_mcs(brw, mt,
> is_lossless_compressed);
>}
> +   } else {
> +  mt->aux_disable |= INTEL_AUX_DISABLE_CCS;
> }
>
> return mt;
> @@ -803,7 +806,7 @@ create_ccs_buf_for_image(struct brw_context *intel,
> mt->mcs_buf->qpitch = isl_surf_get_array_pitch_sa_
> rows(_ccs_surf);
>
> intel_miptree_init_mcs(intel, mt, 0);
> -   mt->aux_disable &= ~INTEL_AUX_DISABLE_CCS;
> +   assert(!(mt->aux_disable & INTEL_AUX_DISABLE_CCS));
> mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;
>
> return true;
> @@ -894,18 +897,19 @@ 

[Mesa-dev] [PATCH v13 21/36] i965: Restructure CCS disabling

[Daniel Stone]

From: Ben Widawsky 

Make the code only disable CCS when it has to, unlike before where it
disabled CCS and enabled it when it could. This is much more inline with
how it should work in a few patches, where we have fewer restrictions as
to when we disable CCS.

v2: Change CCS disabling to an assertion in layout creation (Topi)

v3: Make sure to disable aux buffers when creating a miptree from a BO.
Today, this only happens via intel_update_image_buffer. At the end of
the modifier series, we should be able to undo this. Some fixes from
Topi in here as well.

Cc: "Pohjolainen, Topi" 
Signed-off-by: Ben Widawsky 

Topi's changes

Signed-off-by: Daniel Stone 
---
 src/mesa/drivers/dri/i965/brw_context.c   |  3 +++
 src/mesa/drivers/dri/i965/intel_fbo.h |  7 +++
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 22 +-
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 5055dd76a8..1a2b64f73e 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1718,6 +1718,9 @@ intel_update_image_buffer(struct brw_context *intel,
if (last_mt && last_mt->bo == buffer->bo)
   return;
 
+   if (!buffer->aux_offset)
+  rb->no_aux = true;
+
intel_update_winsys_renderbuffer_miptree(intel, rb, buffer->bo,
 buffer->width, buffer->height,
 buffer->pitch);
diff --git a/src/mesa/drivers/dri/i965/intel_fbo.h 
b/src/mesa/drivers/dri/i965/intel_fbo.h
index 08b82e8934..9265aab2e2 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.h
+++ b/src/mesa/drivers/dri/i965/intel_fbo.h
@@ -111,6 +111,13 @@ struct intel_renderbuffer
 * for the duration of a mapping.
 */
bool singlesample_mt_is_tmp;
+
+   /**
+* Set to true if this buffer definitely does not have auxiliary data, like
+* CCS, associated with it. It's generally to be used when importing a
+* DRIimage, where that DRIimage had no modifier.
+*/
+   bool no_aux;
 };
 
 
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index a8564d9573..9dca5cc435 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -328,7 +328,6 @@ intel_miptree_create_layout(struct brw_context *brw,
mt->logical_depth0 = depth0;
mt->aux_disable = (layout_flags & MIPTREE_LAYOUT_DISABLE_AUX) != 0 ?
   INTEL_AUX_DISABLE_ALL : INTEL_AUX_DISABLE_NONE;
-   mt->aux_disable |= INTEL_AUX_DISABLE_CCS;
mt->is_scanout = (layout_flags & MIPTREE_LAYOUT_FOR_SCANOUT) != 0;
exec_list_make_empty(>hiz_map);
exec_list_make_empty(>color_resolve_map);
@@ -521,6 +520,8 @@ intel_miptree_create_layout(struct brw_context *brw,
} else if (brw->gen >= 9 && num_samples > 1) {
   layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
} else {
+  mt->aux_disable |= INTEL_AUX_DISABLE_CCS;
+
   const UNUSED bool is_lossless_compressed_aux =
  brw->gen >= 9 && num_samples == 1 &&
  mt->format == MESA_FORMAT_R_UINT32;
@@ -696,7 +697,6 @@ intel_miptree_create(struct brw_context *brw,
 */
if (intel_tiling_supports_non_msrt_mcs(brw, mt->tiling) &&
intel_miptree_supports_non_msrt_fast_clear(brw, mt)) {
-  mt->aux_disable &= ~INTEL_AUX_DISABLE_CCS;
   assert(brw->gen < 8 || mt->halign == 16 || num_samples <= 1);
 
   /* On Gen9+ clients are not currently capable of consuming compressed
@@ -710,8 +710,11 @@ intel_miptree_create(struct brw_context *brw,
  intel_miptree_supports_lossless_compressed(brw, mt);
 
   if (is_lossless_compressed) {
+ assert((mt->aux_disable & INTEL_AUX_DISABLE_CCS) == 0);
  intel_miptree_alloc_non_msrt_mcs(brw, mt, is_lossless_compressed);
   }
+   } else {
+  mt->aux_disable |= INTEL_AUX_DISABLE_CCS;
}
 
return mt;
@@ -803,7 +806,7 @@ create_ccs_buf_for_image(struct brw_context *intel,
mt->mcs_buf->qpitch = isl_surf_get_array_pitch_sa_rows(_ccs_surf);
 
intel_miptree_init_mcs(intel, mt, 0);
-   mt->aux_disable &= ~INTEL_AUX_DISABLE_CCS;
+   assert(!(mt->aux_disable & INTEL_AUX_DISABLE_CCS));
mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;
 
return true;
@@ -894,18 +897,19 @@ intel_update_winsys_renderbuffer_miptree(struct 
brw_context *intel,
  height,
  1,
  pitch,
- MIPTREE_LAYOUT_FOR_SCANOUT);
+ MIPTREE_LAYOUT_FOR_SCANOUT |
+ irb->no_aux ? 
MIPTREE_LAYOUT_DISABLE_AUX: 0);
if (!singlesample_mt)
   goto