[Mesa-dev] [PATCH 2/8] i965/gen6: Use isl for hiz

Jason Ekstrand jason at jlekstrand.net
Thu Jun 15 23:50:37 UTC 2017


On Wed, Jun 14, 2017 at 12:55 PM, Pohjolainen, Topi <
topi.pohjolainen at gmail.com> wrote:

> On Wed, Jun 14, 2017 at 10:18:18AM +0300, Pohjolainen, Topi wrote:
> > On Tue, Jun 13, 2017 at 04:20:02PM -0700, Jason Ekstrand wrote:
> > > On Tue, Jun 13, 2017 at 4:14 PM, Jason Ekstrand <jason at jlekstrand.net>
> > > wrote:
> > >
> > > > On Tue, Jun 13, 2017 at 7:53 AM, Topi Pohjolainen <
> > > > topi.pohjolainen at gmail.com> wrote:
> > > >
> > > >> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > >> ---
> > > >>  src/mesa/drivers/dri/i965/brw_blorp.c         |  9 +++--
> > > >>  src/mesa/drivers/dri/i965/gen6_depth_state.c  | 12 +++----
> > > >>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 50
> > > >> ++++++++++++++-------------
> > > >>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  7 ++--
> > > >>  4 files changed, 39 insertions(+), 39 deletions(-)
> > > >>
> > > >> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > >> b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > >> index 4bc53b76b5..b722454703 100644
> > > >> --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > >> +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > >> @@ -165,8 +165,13 @@ blorp_surf_for_miptree(struct brw_context *brw,
> > > >>
> > > >>     surf->aux_usage = intel_miptree_get_aux_isl_usage(brw, mt);
> > > >>
> > > >> -   struct isl_surf *aux_surf = &tmp_surfs[1];
> > > >> -   intel_miptree_get_aux_isl_surf(brw, mt, surf->aux_usage,
> aux_surf);
> > > >> +   struct isl_surf *aux_surf;
> > > >> +   if (brw->gen == 6 && mt->hiz_buf) {
> > > >> +      aux_surf = &mt->hiz_buf->aux_base.surf;
> > > >> +   } else {
> > > >> +      aux_surf = &tmp_surfs[1];
> > > >> +      intel_miptree_get_aux_isl_surf(brw, mt, surf->aux_usage,
> > > >> aux_surf);
> > > >>
> > > >
> > > > This is a bit awkward.  Maybe just make
> intel_miptree_get_aux_isl_surf
> > > > return the surf from hiz_buf on gen6?  Not that it matters much
> since I
> > > > have a feeling this is all going away in the future.
> >
> > I'd like to keep intel_miptree_get_aux_isl_surf() unchanged, I'm
> throwing it
> > out later and it is clearer when I don't need to move anything back from
> it.
> >
> > > >
> > > >
> > > >> +   }
> > > >>
> > > >>     if (wants_resolve) {
> > > >>        bool supports_aux = surf->aux_usage != ISL_AUX_USAGE_NONE &&
> > > >> diff --git a/src/mesa/drivers/dri/i965/gen6_depth_state.c
> > > >> b/src/mesa/drivers/dri/i965/gen6_depth_state.c
> > > >> index 0d8785db65..0f5e4d3201 100644
> > > >> --- a/src/mesa/drivers/dri/i965/gen6_depth_state.c
> > > >> +++ b/src/mesa/drivers/dri/i965/gen6_depth_state.c
> > > >> @@ -165,18 +165,14 @@ gen6_emit_depth_stencil_hiz(struct
> brw_context
> > > >> *brw,
> > > >>        /* Emit hiz buffer. */
> > > >>        if (hiz) {
> > > >>           assert(depth_mt);
> > > >> -         struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_buf->mt;
> > > >>
> > > >> -         assert(hiz_mt->array_layout == GEN6_HIZ_STENCIL);
> > > >> -
> > > >> -         const uint32_t offset = intel_miptree_get_aligned_offset(
> > > >> -                                    hiz_mt,
> > > >> -                                    hiz_mt->level[lod].level_x,
> > > >> -                                    hiz_mt->level[lod].level_y);
> > > >> +         uint32_t offset;
> > > >> +         isl_surf_get_image_offset_B_tile_sa(&depth_mt->hiz_buf->
> aux
> > > >> _base.surf,
> > > >> +                                             lod, 0, 0, &offset,
> NULL,
> > > >> NULL);
> > > >>
> > > >>          BEGIN_BATCH(3);
> > > >>          OUT_BATCH((_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2));
> > > >> -        OUT_BATCH(depth_mt->hiz_buf->aux_base.pitch - 1);
> > > >> +        OUT_BATCH(depth_mt->hiz_buf->aux_base.surf.row_pitch - 1);
> > > >>          OUT_RELOC(depth_mt->hiz_buf->aux_base.bo,
> > > >>                    I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER,
> > > >>                    offset);
> > > >> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > >> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > >> index d87dbfaacd..4dbf853eee 100644
> > > >> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > >> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > >> @@ -1058,10 +1058,7 @@ intel_miptree_hiz_buffer_free(struct
> > > >> intel_miptree_hiz_buffer *hiz_buf)
> > > >>     if (hiz_buf == NULL)
> > > >>        return;
> > > >>
> > > >> -   if (hiz_buf->mt)
> > > >> -      intel_miptree_release(&hiz_buf->mt);
> > > >> -   else
> > > >> -      brw_bo_unreference(hiz_buf->aux_base.bo);
> > > >> +   brw_bo_unreference(hiz_buf->aux_base.bo);
> > > >>
> > > >>     free(hiz_buf);
> > > >>  }
> > > >> @@ -2007,34 +2004,39 @@ intel_hiz_miptree_buf_create(struct
> brw_context
> > > >> *brw,
> > > >>                               struct intel_mipmap_tree *mt)
> > > >>  {
> > > >>     struct intel_miptree_hiz_buffer *buf = calloc(sizeof(*buf), 1);
> > > >> -   uint32_t layout_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD;
> > > >> +   if (!buf)
> > > >> +      return NULL;
> > > >>
> > > >> -   if (brw->gen == 6)
> > > >> -      layout_flags |= MIPTREE_LAYOUT_GEN6_HIZ_STENCIL;
> > > >> +   struct isl_surf_init_info init_info = {
> > > >> +      .dim = get_isl_surf_dim(mt->target),
> > > >> +      .format = ISL_FORMAT_HIZ,
> > > >> +      .width = mt->logical_width0,
> > > >> +      .height = mt->logical_height0,
> > > >> +      .depth = mt->target == GL_TEXTURE_3D ? mt->logical_depth0 :
> 1,
> > > >> +      .levels = mt->last_level - mt->first_level + 1,
> > > >> +      .array_len = mt->target == GL_TEXTURE_3D ? 1 :
> mt->logical_depth0,
> > > >> +      .samples = MAX2(mt->num_samples, 1),
> > > >> +      .usage = ISL_SURF_USAGE_HIZ_BIT,
> > > >> +      .tiling_flags = ISL_TILING_HIZ_BIT
> > > >>
> > > >
> > > Is there some reason why you can't just use miptree_get_isl_surf() and
> > > isl_surf_get_hiz_surf()?  That seems like the more straightforward
> path.
> >
> > This is a good call as well. This was needed when I had to create layout
> > for level zero only and tweak it later on. Now that you gave me proper
> support
> > in ISL, this is not needed anymore.
>
> I would need to create temporary main surface though in order to use
> isl_surf_get_hiz_surf(). At this point I don't have it yet. Would you be
> okay
> changing this after the series that changes depth to use isl?
>

I'd prefer the temp but I guess I won't insist too hard.  I would just feel
better if all HiZ surface creation were going to isl_surf_get_hiz_surf().

--Jason


> >
> > >
> > >
> > > > +   };
> > > >>
> > > >> -   if (!buf)
> > > >> +   if (!isl_surf_init_s(&brw->isl_dev, &buf->aux_base.surf,
> > > >> &init_info)) {
> > > >> +      free(buf);
> > > >>        return NULL;
> > > >> +   }
> > > >>
> > > >> -   layout_flags |= MIPTREE_LAYOUT_TILING_ANY;
> > > >> -   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,
> > > >> -                                  mt->num_samples,
> > > >> -                                  layout_flags);
> > > >> -   if (!buf->mt) {
> > > >> +   struct isl_surf *surf = &buf->aux_base.surf;
> > > >> +   unsigned pitch = surf->row_pitch;
> > > >> +   buf->aux_base.bo = brw_bo_alloc_tiled(brw->bufmgr, "hiz",
> > > >> surf->row_pitch,
> > > >> +                                         surf->size /
> surf->row_pitch, 1,
> > > >> +                                         I915_TILING_Y, &pitch,
> > > >> +                                         BO_ALLOC_FOR_RENDER);
> > > >>
> > > >
> > > I just sent 3 patches to the list which make this less painful. :-)
> > >
> > >
> > > > +   if (!buf->aux_base.bo) {
> > > >>        free(buf);
> > > >>        return NULL;
> > > >>     }
> > > >>
> > > >> -   buf->aux_base.bo = buf->mt->bo;
> > > >> -   buf->aux_base.size = buf->mt->total_height * buf->mt->pitch;
> > > >> -   buf->aux_base.pitch = buf->mt->pitch;
> > > >> -   buf->aux_base.qpitch = buf->mt->qpitch * 2;
> > > >> +   assert(pitch == surf->row_pitch);
> > > >>
> > > >>     return buf;
> > > >>  }
> > > >> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > >> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > >> index 37d6bb3813..9f23088d7b 100644
> > > >> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > >> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > >> @@ -307,6 +307,8 @@ enum intel_aux_disable {
> > > >>   */
> > > >>  struct intel_miptree_aux_buffer
> > > >>  {
> > > >> +   struct isl_surf surf;
> > > >> +
> > > >>     /**
> > > >>      * Buffer object containing the pixel data.
> > > >>      *
> > > >> @@ -360,11 +362,6 @@ struct intel_miptree_aux_buffer
> > > >>  struct intel_miptree_hiz_buffer
> > > >>  {
> > > >>     struct intel_miptree_aux_buffer aux_base;
> > > >> -
> > > >> -   /**
> > > >> -    * Hiz miptree. Used only by Gen6.
> > > >> -    */
> > > >> -   struct intel_mipmap_tree *mt;
> > > >>  };
> > > >>
> > > >>  struct intel_mipmap_tree
> > > >> --
> > > >> 2.11.0
> > > >>
> > > >> _______________________________________________
> > > >> mesa-dev mailing list
> > > >> mesa-dev at lists.freedesktop.org
> > > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >>
> > > >
> > > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170615/a9b80d77/attachment-0001.html>


More information about the mesa-dev mailing list