[Mesa-dev] [PATCH v2 01/14] i965/hiz: Start to separate miptree out from hiz buffers

Ben Widawsky ben at bwidawsk.net
Mon Nov 10 10:42:16 PST 2014


On Mon, Jul 21, 2014 at 11:00:50PM -0700, Jordan Justen wrote:
> Today we allocate a miptree's for the hiz buffer. We needed this in
> the past because we would point the hardware at offsets of the hiz
> buffer. Since the hiz format is not documented, this is not a good
> idea.
> 
> Since moving to support layered rendering on Gen7+, we no longer point
> at an offset into the buffer on Gen7+.
> 
> Therefore, to support hiz on Gen7+, we don't need a full miptree
> structure allocated.
> 
> This patch starts to create a new auxiliary buffer structure
> (intel_miptree_aux_buffer) that can be a more simplistic miptree
> side-band buffer associated with a miptree. (For example, to serve the
> needs of the hiz buffer.)
> 
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_misc_state.c    |  4 +-
>  src/mesa/drivers/dri/i965/gen6_blorp.cpp      |  2 +-
>  src/mesa/drivers/dri/i965/gen7_blorp.cpp      |  2 +-
>  src/mesa/drivers/dri/i965/gen7_misc_state.c   |  2 +-
>  src/mesa/drivers/dri/i965/gen8_depth_state.c  |  6 +--
>  src/mesa/drivers/dri/i965/intel_fbo.c         |  4 +-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 55 +++++++++++++++++++--------
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 29 ++++++++++++--
>  8 files changed, 76 insertions(+), 28 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/dri/i965/brw_misc_state.c
> index 76e22bd..af900cd 100644
> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> @@ -182,7 +182,7 @@ brw_get_depthstencil_tile_masks(struct intel_mipmap_tree *depth_mt,
>  
>        if (intel_miptree_level_has_hiz(depth_mt, depth_level)) {
>           uint32_t hiz_tile_mask_x, hiz_tile_mask_y;
> -         intel_miptree_get_tile_masks(depth_mt->hiz_mt,
> +         intel_miptree_get_tile_masks(depth_mt->hiz_buf->mt,
>                                        &hiz_tile_mask_x, &hiz_tile_mask_y,
>                                        false);
>  
> @@ -643,7 +643,7 @@ brw_emit_depth_stencil_hiz(struct brw_context *brw,
>  
>        /* Emit hiz buffer. */
>        if (hiz) {
> -         struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_mt;
> +         struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_buf->mt;
>  	 BEGIN_BATCH(3);
>  	 OUT_BATCH((_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2));
>  	 OUT_BATCH(hiz_mt->pitch - 1);
> diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> index eb865b9..282c7b2 100644
> --- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> +++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> @@ -855,7 +855,7 @@ gen6_blorp_emit_depth_stencil_config(struct brw_context *brw,
>  
>     /* 3DSTATE_HIER_DEPTH_BUFFER */
>     {
> -      struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_mt;
> +      struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_buf->mt;
>        uint32_t hiz_offset =
>           intel_miptree_get_aligned_offset(hiz_mt,
>                                            draw_x & ~tile_mask_x,
> diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> index 0ad570b..d0d623d 100644
> --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> @@ -752,7 +752,7 @@ gen7_blorp_emit_depth_stencil_config(struct brw_context *brw,
>  
>     /* 3DSTATE_HIER_DEPTH_BUFFER */
>     {
> -      struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_mt;
> +      struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_buf->mt;
>  
>        BEGIN_BATCH(3);
>        OUT_BATCH((GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2));
> diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> index 22911bf..6c6a79b 100644
> --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> @@ -145,7 +145,7 @@ gen7_emit_depth_stencil_hiz(struct brw_context *brw,
>        OUT_BATCH(0);
>        ADVANCE_BATCH();
>     } else {
> -      struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_mt;
> +      struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_buf->mt;
>        BEGIN_BATCH(3);
>        OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (3 - 2));
>        OUT_BATCH((mocs << 25) |
> diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> index 7c3bfe0..b6f373d 100644
> --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> @@ -89,10 +89,10 @@ emit_depth_packets(struct brw_context *brw,
>     } else {
>        BEGIN_BATCH(5);
>        OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (5 - 2));
> -      OUT_BATCH((depth_mt->hiz_mt->pitch - 1) | BDW_MOCS_WB << 25);
> -      OUT_RELOC64(depth_mt->hiz_mt->bo,
> +      OUT_BATCH((depth_mt->hiz_buf->mt->pitch - 1) | BDW_MOCS_WB << 25);
> +      OUT_RELOC64(depth_mt->hiz_buf->mt->bo,
>                    I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> -      OUT_BATCH(depth_mt->hiz_mt->qpitch >> 2);
> +      OUT_BATCH(depth_mt->hiz_buf->mt->qpitch >> 2);
>        ADVANCE_BATCH();
>     }
>  
> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
> index e43e18b..7f9964c 100644
> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> @@ -560,9 +560,9 @@ intel_renderbuffer_update_wrapper(struct brw_context *brw,
>  
>     intel_renderbuffer_set_draw_offset(irb);
>  
> -   if (mt->hiz_mt == NULL && brw_is_hiz_depth_format(brw, rb->Format)) {
> +   if (mt->hiz_buf == NULL && brw_is_hiz_depth_format(brw, rb->Format)) {
>        intel_miptree_alloc_hiz(brw, mt);
> -      if (!mt->hiz_mt)
> +      if (!mt->hiz_buf)
>  	 return false;
>     }
>  
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index b36ffc7..d5a2227 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -822,7 +822,10 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
>  
>        drm_intel_bo_unreference((*mt)->bo);
>        intel_miptree_release(&(*mt)->stencil_mt);
> -      intel_miptree_release(&(*mt)->hiz_mt);
> +      if ((*mt)->hiz_buf) {

In addition, I would have put:
assert(&(*mt)->hiz_buf->mt)

> +         intel_miptree_release(&(*mt)->hiz_buf->mt);
> +         free((*mt)->hiz_buf);
> +      }
>        intel_miptree_release(&(*mt)->mcs_mt);
>        intel_resolve_map_clear(&(*mt)->hiz_map);
>  
> @@ -1347,7 +1350,7 @@ intel_miptree_level_enable_hiz(struct brw_context *brw,
>                                 struct intel_mipmap_tree *mt,
>                                 uint32_t level)
>  {
> -   assert(mt->hiz_mt);
> +   assert(mt->hiz_buf);
>  
>     if (brw->gen >= 8 || brw->is_haswell) {
>        uint32_t width = minify(mt->physical_width0, level);
> @@ -1371,25 +1374,47 @@ intel_miptree_level_enable_hiz(struct brw_context *brw,
>  }
>  
>  
> +static struct intel_miptree_aux_buffer *
> +intel_hiz_miptree_buf_create(struct brw_context *brw,
> +                             struct intel_mipmap_tree *mt)
> +{
> +   struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1);
> +
> +   if (!buf)
> +      return NULL;
> +
> +   buf->mt = intel_miptree_create(brw,
> +                                  mt->target,
> +                                  mt->format,
> +                                  mt->first_level,
> +                                  mt->last_level,
> +                                  mt->logical_width0,
> +                                  mt->logical_height0,
> +                                  mt->logical_depth0,
> +                                  true,
> +                                  mt->num_samples,
> +                                  INTEL_MIPTREE_TILING_ANY);
> +   if (!buf->mt) {
> +      free(buf);
> +      return NULL;
> +   }
> +
> +   buf->bo = buf->mt->bo;
> +   buf->pitch = buf->mt->pitch;
> +   buf->qpitch = buf->mt->qpitch;
> +
> +   return buf;
> +}
> +
>  
>  bool
>  intel_miptree_alloc_hiz(struct brw_context *brw,
>  			struct intel_mipmap_tree *mt)
>  {
> -   assert(mt->hiz_mt == NULL);
> -   mt->hiz_mt = intel_miptree_create(brw,
> -                                     mt->target,
> -                                     mt->format,
> -                                     mt->first_level,
> -                                     mt->last_level,
> -                                     mt->logical_width0,
> -                                     mt->logical_height0,
> -                                     mt->logical_depth0,
> -                                     true,
> -                                     mt->num_samples,
> -                                     INTEL_MIPTREE_TILING_ANY);
> +   assert(mt->hiz_buf == NULL);
> +   mt->hiz_buf = intel_hiz_miptree_buf_create(brw, mt);
>  
> -   if (!mt->hiz_mt)
> +   if (!mt->hiz_buf)
>        return false;
>  
>     /* Mark that all slices need a HiZ resolve. */
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index bb04084..2a421a1 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -255,6 +255,29 @@ enum intel_fast_clear_state
>     INTEL_FAST_CLEAR_STATE_CLEAR,
>  };
>  
> +/**
> + * Miptree aux buffer. These buffers are associated with a miptree, but the
> + * format is managed by the hardware.
> + *
> + * For Gen7+, we always give the hardware the start of the buffer, and let it
> + * handle all accesses to the buffer. Therefore we don't need the full miptree
> + * layout structure for this buffer.
> + *
> + * For Gen6, we need a hiz miptree structure for this buffer so we can program
> + * offsets to slices & miplevels.
> + */
> +struct intel_miptree_aux_buffer
> +{
> +   /** Buffer object containing the pixel data. */
> +   drm_intel_bo *bo;
> +
> +   uint32_t pitch; /**< pitch in bytes. */
> +
> +   uint32_t qpitch; /**< The distance in rows between array slices. */
> +
> +   struct intel_mipmap_tree *mt; /**< hiz miptree used with Gen6 */
> +};
> +

Doesn't the needs resolve flag belong here too?

>  struct intel_mipmap_tree
>  {
>     /** Buffer object containing the pixel data. */
> @@ -362,15 +385,15 @@ struct intel_mipmap_tree
>     uint32_t offset;
>  
>     /**
> -    * \brief HiZ miptree
> +    * \brief HiZ aux buffer
>      *
>      * The hiz miptree contains the miptree's hiz buffer. To allocate the hiz
> -    * miptree, use intel_miptree_alloc_hiz().
> +    * buffer, use intel_miptree_alloc_hiz().
>      *
>      * To determine if hiz is enabled, do not check this pointer. Instead, use
>      * intel_miptree_slice_has_hiz().
>      */
> -   struct intel_mipmap_tree *hiz_mt;
> +   struct intel_miptree_aux_buffer *hiz_buf;

>  
>     /**
>      * \brief Map of miptree slices to needed resolves.
> -- 

Just some additional notes:
Given that the series is driving towards AUX buffer support, and you
mention "aux" in the comment, maybe it's time to rename this something
other than "hiz"?

Also, as a cheap transitional hack you could have probably done
#define hiz_mt hiz_buf->mt
That would have removed a bunch of LOC.

I see nothing wrong.
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center


More information about the mesa-dev mailing list