On Mon, Nov 07, 2016 at 12:30:14PM +0200, Pohjolainen, Topi wrote:
> On Mon, Nov 07, 2016 at 10:24:35AM +0000, Lionel Landwerlin wrote:
> > On 07/11/16 10:18, Pohjolainen, Topi wrote:
> > > On Thu, Nov 03, 2016 at 10:39:40AM +0000, Lionel Landwerlin wrote:
> > > > From: Ben Widawsky <benjamin.widaw...@intel.com>
> > > >
> > > > This seems counter to the goal of consolidating hiz, mcs, and later ccs
> > > > buffers.
> > > > Unfortunately, hiz on gen6 is a thing the code supports, and this wart
> > > > will be
> > > > helpful to achieve that. Overall, I believe it does help unify AUX
> > > > buffers on
> > > > gen7+.
> > > >
> > > > I updated the size field which I introduced in the previous patch, even
> > > > though
> > > > we have no use for it.
> > > >
> > > > XXX: As I mentioned in the last patch, the height given to the MCS
> > > > buffer
> > > > allocation in intel_miptree_alloc_mcs() looks wrong, but I don't claim
> > > > to fully
> > > > understand how the MCS buffer is laid out.
> > > >
> > > > v2: rebase on master (Lionel)
> > > >
> > > > Signed-off-by: Ben Widawsky <benjamin.widaw...@intel.com> (v1)
> > > > Signed-off-by: Lionel Landwerlin <lionel.g.landwer...@intel.com> (v2)
> > > > ---
> > > > src/mesa/drivers/dri/i965/brw_blorp.c | 4 +-
> > > > src/mesa/drivers/dri/i965/gen7_misc_state.c | 6 +--
> > > > src/mesa/drivers/dri/i965/gen8_depth_state.c | 6 +--
> > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 61
> > > > ++++++++++++++-------------
> > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 14 ++++--
> > > > 5 files changed, 49 insertions(+), 42 deletions(-)
> > > >
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > index 5adb4c6..f0ad074 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> > > > @@ -214,8 +214,8 @@ blorp_surf_for_miptree(struct brw_context *brw,
> > > > }
> > > > assert(hiz_mt->pitch == aux_surf->row_pitch);
> > > > } else {
> > > > - surf->aux_addr.buffer = mt->hiz_buf->bo;
> > > > - surf->aux_addr.offset = 0;
> > > > + surf->aux_addr.buffer = mt->hiz_buf->aux_base.bo;
> > > > + surf->aux_addr.offset = mt->hiz_buf->aux_base.offset;
> > > > }
> > > > }
> > > > } else {
> > > > diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > > b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > > index 271d962..7bd5cd5 100644
> > > > --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > > +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c
> > > > @@ -146,13 +146,13 @@ gen7_emit_depth_stencil_hiz(struct brw_context
> > > > *brw,
> > > > ADVANCE_BATCH();
> > > > } else {
> > > > assert(depth_mt);
> > > > - struct intel_miptree_aux_buffer *hiz_buf = depth_mt->hiz_buf;
> > > > + struct intel_miptree_hiz_buffer *hiz_buf = depth_mt->hiz_buf;
> > > > BEGIN_BATCH(3);
> > > > OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (3 - 2));
> > > > OUT_BATCH((mocs << 25) |
> > > > - (hiz_buf->pitch - 1));
> > > > - OUT_RELOC(hiz_buf->bo,
> > > > + (hiz_buf->aux_base.pitch - 1));
> > > > + OUT_RELOC(hiz_buf->aux_base.bo,
> > > > I915_GEM_DOMAIN_RENDER,
> > > > I915_GEM_DOMAIN_RENDER,
> > > > 0);
> > > > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > > b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > > index 73b2186..8920910 100644
> > > > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c
> > > > @@ -93,10 +93,10 @@ emit_depth_packets(struct brw_context *brw,
> > > > assert(depth_mt);
> > > > BEGIN_BATCH(5);
> > > > OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (5 - 2));
> > > > - OUT_BATCH((depth_mt->hiz_buf->pitch - 1) | mocs_wb << 25);
> > > > - OUT_RELOC64(depth_mt->hiz_buf->bo,
> > > > + OUT_BATCH((depth_mt->hiz_buf->aux_base.pitch - 1) | mocs_wb <<
> > > > 25);
> > > > + OUT_RELOC64(depth_mt->hiz_buf->aux_base.bo,
> > > > I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> > > > - OUT_BATCH(depth_mt->hiz_buf->qpitch >> 2);
> > > > + OUT_BATCH(depth_mt->hiz_buf->aux_base.qpitch >> 2);
> > > > ADVANCE_BATCH();
> > > > }
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > index 3d1bdb1..af0e1a4 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > @@ -1016,11 +1016,10 @@ intel_miptree_release(struct intel_mipmap_tree
> > > > **mt)
> > > > if ((*mt)->hiz_buf->mt)
> > > > intel_miptree_release(&(*mt)->hiz_buf->mt);
> > > > else
> > > > - drm_intel_bo_unreference((*mt)->hiz_buf->bo);
> > > > + drm_intel_bo_unreference((*mt)->hiz_buf->aux_base.bo);
> > > > free((*mt)->hiz_buf);
> > > > }
> > > > if ((*mt)->mcs_buf) {
> > > > - intel_miptree_release(&(*mt)->mcs_buf->mt);
> > > Doesn't this belong to the one of the earlier patches?
> > >
> > > > free((*mt)->mcs_buf);
> > > > }
> > > > intel_resolve_map_clear(&(*mt)->hiz_map);
> > > > @@ -1726,7 +1725,7 @@ intel_miptree_level_enable_hiz(struct brw_context
> > > > *brw,
> > > > * Helper for intel_miptree_alloc_hiz() that determines the required
> > > > hiz
> > > > * buffer dimensions and allocates a bo for the hiz buffer.
> > > > */
> > > > -static struct intel_miptree_aux_buffer *
> > > > +static struct intel_miptree_hiz_buffer *
> > > > intel_gen7_hiz_buf_create(struct brw_context *brw,
> > > > struct intel_mipmap_tree *mt)
> > > > {
> > > > @@ -1734,7 +1733,7 @@ intel_gen7_hiz_buf_create(struct brw_context *brw,
> > > > unsigned z_height = mt->logical_height0;
> > > > const unsigned z_depth = MAX2(mt->logical_depth0, 1);
> > > > unsigned hz_width, hz_height;
> > > > - struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1);
> > > > + struct intel_miptree_hiz_buffer *buf = calloc(sizeof(*buf), 1);
> > > > if (!buf)
> > > > return NULL;
> > > > @@ -1791,20 +1790,21 @@ intel_gen7_hiz_buf_create(struct brw_context
> > > > *brw,
> > > > unsigned long pitch;
> > > > uint32_t tiling = I915_TILING_Y;
> > > > - buf->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "hiz",
> > > > - hz_width, hz_height, 1,
> > > > - &tiling, &pitch,
> > > > - BO_ALLOC_FOR_RENDER);
> > > > - if (!buf->bo) {
> > > > + buf->aux_base.bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "hiz",
> > > > + hz_width, hz_height, 1,
> > > > + &tiling, &pitch,
> > > > + BO_ALLOC_FOR_RENDER);
> > > > + if (!buf->aux_base.bo) {
> > > > free(buf);
> > > > return NULL;
> > > > } else if (tiling != I915_TILING_Y) {
> > > > - drm_intel_bo_unreference(buf->bo);
> > > > + drm_intel_bo_unreference(buf->aux_base.bo);
> > > > free(buf);
> > > > return NULL;
> > > > }
> > > > - buf->pitch = pitch;
> > > > + buf->aux_base.size = hz_width * hz_height;
> > > > + buf->aux_base.pitch = pitch;
> > > > return buf;
> > > > }
> > > > @@ -1814,7 +1814,7 @@ intel_gen7_hiz_buf_create(struct brw_context *brw,
> > > > * Helper for intel_miptree_alloc_hiz() that determines the required
> > > > hiz
> > > > * buffer dimensions and allocates a bo for the hiz buffer.
> > > > */
> > > > -static struct intel_miptree_aux_buffer *
> > > > +static struct intel_miptree_hiz_buffer *
> > > > intel_gen8_hiz_buf_create(struct brw_context *brw,
> > > > struct intel_mipmap_tree *mt)
> > > > {
> > > > @@ -1822,7 +1822,7 @@ intel_gen8_hiz_buf_create(struct brw_context *brw,
> > > > unsigned z_height = mt->logical_height0;
> > > > const unsigned z_depth = MAX2(mt->logical_depth0, 1);
> > > > unsigned hz_width, hz_height;
> > > > - struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1);
> > > > + struct intel_miptree_hiz_buffer *buf = calloc(sizeof(*buf), 1);
> > > > if (!buf)
> > > > return NULL;
> > > > @@ -1875,42 +1875,43 @@ intel_gen8_hiz_buf_create(struct brw_context
> > > > *brw,
> > > > Z_i = minify(Z_i, 1);
> > > > }
> > > > /* HZ_QPitch = h0 + max(h1, sum(i=2 to m; h_i)) */
> > > > - buf->qpitch = h0 + MAX2(h1, sum_h_i);
> > > > + buf->aux_base.qpitch = h0 + MAX2(h1, sum_h_i);
> > > > if (mt->target == GL_TEXTURE_3D) {
> > > > /* (1/2) * sum(i=0 to m; h_i * max(1, floor(Z_Depth/2**i))) */
> > > > hz_height = DIV_ROUND_UP(hz_height_3d_sum, 2);
> > > > } else {
> > > > /* HZ_Height (rows) = ceiling( (HZ_QPitch/2)/8) *8 * Z_Depth */
> > > > - hz_height = DIV_ROUND_UP(buf->qpitch, 2 * 8) * 8 * Z0;
> > > > + hz_height = DIV_ROUND_UP(buf->aux_base.qpitch, 2 * 8) * 8 * Z0;
> > > > }
> > > > unsigned long pitch;
> > > > uint32_t tiling = I915_TILING_Y;
> > > > - buf->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "hiz",
> > > > - hz_width, hz_height, 1,
> > > > - &tiling, &pitch,
> > > > - BO_ALLOC_FOR_RENDER);
> > > > - if (!buf->bo) {
> > > > + buf->aux_base.bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "hiz",
> > > > + hz_width, hz_height, 1,
> > > > + &tiling, &pitch,
> > > > + BO_ALLOC_FOR_RENDER);
> > > > + if (!buf->aux_base.bo) {
> > > > free(buf);
> > > > return NULL;
> > > > } else if (tiling != I915_TILING_Y) {
> > > > - drm_intel_bo_unreference(buf->bo);
> > > > + drm_intel_bo_unreference(buf->aux_base.bo);
> > > > free(buf);
> > > > return NULL;
> > > > }
> > > > - buf->pitch = pitch;
> > > > + buf->aux_base.size = hz_width * hz_height;
> > > > + buf->aux_base.pitch = pitch;
> > > > return buf;
> > > > }
> > > > -static struct intel_miptree_aux_buffer *
> > > > +static struct intel_miptree_hiz_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);
> > > > + struct intel_miptree_hiz_buffer *buf = calloc(sizeof(*buf), 1);
> > > > uint32_t layout_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD;
> > > > if (brw->gen == 6)
> > > > @@ -1935,9 +1936,10 @@ intel_hiz_miptree_buf_create(struct brw_context
> > > > *brw,
> > > > return NULL;
> > > > }
> > > > - buf->bo = buf->mt->bo;
> > > > - buf->pitch = buf->mt->pitch;
> > > > - buf->qpitch = buf->mt->qpitch;
> > > > + 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;
> > > > return buf;
> > > > }
> > > > @@ -2183,7 +2185,6 @@ intel_miptree_make_shareable(struct brw_context
> > > > *brw,
> > > > if (mt->mcs_buf) {
> > > > intel_miptree_resolve_color(brw, mt, 0);
> > > > - intel_miptree_release(&mt->mcs_buf->mt);
> > > And this, belongs to earlier patch in the series?
> >
> > Thanks again, indeed, should be in patch 3.
>
> Patches 3 and 4 squashed + the two intel_miptree_release() calls here:
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
I was a little hesitant because previous patch introduced
intel_miptree_aux_buffer::size but taking hiz into account. But, hiz still
keeps on using miptree and having one patch addressing mcs and another hiz
looks rather clean. This patch is also:
Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>
> >
> > >
> > > > mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_NO_MCS;
> > > > }
> > > > }
> > > > @@ -3274,8 +3275,8 @@ intel_miptree_get_aux_isl_surf(struct brw_context
> > > > *brw,
> > > > aux_pitch = mt->hiz_buf->mt->pitch;
> > > > aux_qpitch = mt->hiz_buf->mt->qpitch;
> > > > } else {
> > > > - aux_pitch = mt->hiz_buf->pitch;
> > > > - aux_qpitch = mt->hiz_buf->qpitch;
> > > > + aux_pitch = mt->hiz_buf->aux_base.pitch;
> > > > + aux_qpitch = mt->hiz_buf->aux_base.qpitch;
> > > > }
> > > > *usage = ISL_AUX_USAGE_HIZ;
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > index 0b4b353..e9024a1 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > @@ -324,9 +324,6 @@ enum miptree_array_layout {
> > > > * 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
> > > > {
> > > > @@ -374,6 +371,15 @@ struct intel_miptree_aux_buffer
> > > > * @see 3DSTATE_HIER_DEPTH_BUFFER.SurfaceQPitch
> > > > */
> > > > uint32_t qpitch;
> > > > +};
> > > > +/**
> > > > + * The HiZ buffer requires extra attributes on earlier GENs. This is
> > > > easily
> > > > + * contained within an intel_mipmap_tree. To make sure we do not abuse
> > > > this, we
> > > > + * keep the hiz datastructure separate.
> > > > + */
> > > > +struct intel_miptree_hiz_buffer
> > > > +{
> > > > + struct intel_miptree_aux_buffer aux_base;
> > > > /**
> > > > * Hiz miptree. Used only by Gen6.
> > > > @@ -609,7 +615,7 @@ struct intel_mipmap_tree
> > > > * To determine if hiz is enabled, do not check this pointer.
> > > > Instead, use
> > > > * intel_miptree_slice_has_hiz().
> > > > */
> > > > - struct intel_miptree_aux_buffer *hiz_buf;
> > > > + struct intel_miptree_hiz_buffer *hiz_buf;
> > > > /**
> > > > * \brief Map of miptree slices to needed resolves.
> > > > --
> > > > 2.10.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 mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
