On Tue, Jul 22, 2014 at 02:22:04PM -0700, Jordan Justen wrote:
> On Tue, Jul 22, 2014 at 11:14 AM, Pohjolainen, Topi
> <topi.pohjolai...@intel.com> wrote:
> > On Fri, Jul 18, 2014 at 02:16:47PM -0700, Jordan Justen wrote:
> >> Generalize the name array_spacing_lod0 to non_mip_arrays. Previously
> >> it was only used in certain cases where only a single mip-level was
> >> used.
> >>
> >> For gen6 we will use non-mipmapped array spacing, but with multiple
> >> mip levels. This is needed because gen6 hiz and stencil only support a
> >> single mip-level.
> >>
> >> PRM Volume 1, Part 1, 7.18.3.7.2 For separate stencil buffer [DevILK]
> >> to [DevSNB]:
> >> "The separate stencil buffer does not support mip mapping, thus the
> >> storage for LODs other than LOD 0 is not needed."
> >>
> >> PRM Volume 2, Part 1, 7.5.3 Hierarchical Depth Buffer
> >> "[DevSNB]: The hierarchical depth buffer does not support the LOD
> >> field, it is assumed by hardware to be zero. A separate
> >> hierarachical depth buffer is required for each LOD used, and the
> >> corresponding buffer???s state delivered to hardware each time a new
> >> depth buffer state with modified LOD is delivered."
> >>
> >> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
> >
> > I know I complained about the name in the first place, and I'm not too happy
> > about this either. The structure is still a "mip array" as it supports
> > multiple layers for multiple levels (I think the original as array of
> > miptrees and this new layout as miptree of arrays). I don't have anything
> > better to suggest and hence I'm fine with this or even dropping this patch.
>
> I don't want leave it as array_spacing_lod0, since that has a specific
> hardware meaning, and we are extending the use of the field.
>
> What about something like this?
>
> enum miptree_array_layout {
> /* Each array slice contains all miplevels packed together.
> *
> * Gen hardware usually wants miptrees
> * configured this way.
> */
> ALL_LOD_IN_EACH_SLICE,
>
> /* Each LOD contains all slices of that LOD packed together.
> *
> * Multisampled surfaces use this for array_spacing_lod0.
> *
> * Gen6 uses this for separate stencil and hiz.
> */
> ALL_SLICES_AT_EACH_LOD,
> };
>
> So, code might look like:
> if (mt->array_layout == ALL_SLICES_AT_EACH_LOD)
This looks good to me! Thanks for taking time and thinking it through.
>
> -Jordan
>
> > I had some minor comments for the rest of the series and something to be
> > clarified in patch 16 but 12-17 are:
> >
> > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
> >
> >> ---
> >> src/mesa/drivers/dri/i965/brw_blorp.cpp | 2 +-
> >> src/mesa/drivers/dri/i965/brw_blorp.h | 2 +-
> >> src/mesa/drivers/dri/i965/brw_tex_layout.c | 2 +-
> >> src/mesa/drivers/dri/i965/gen7_blorp.cpp | 2 +-
> >> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 6 +++---
> >> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 6 +++---
> >> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
> >> 7 files changed, 11 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> index b57721c..c5ed84a 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> >> @@ -82,7 +82,7 @@ brw_blorp_surface_info::set(struct brw_context *brw,
> >> {
> >> brw_blorp_mip_info::set(mt, level, layer);
> >> this->num_samples = mt->num_samples;
> >> - this->array_spacing_lod0 = mt->array_spacing_lod0;
> >> + this->non_mip_arrays = mt->non_mip_arrays;
> >> this->map_stencil_as_y_tiled = false;
> >> this->msaa_layout = mt->msaa_layout;
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h
> >> b/src/mesa/drivers/dri/i965/brw_blorp.h
> >> index 683f09e..0b360c5 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_blorp.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_blorp.h
> >> @@ -153,7 +153,7 @@ public:
> >> /* Setting this flag indicates that the surface should be set up in
> >> * ARYSPC_LOD0 mode. Ignored prior to Gen7.
> >> */
> >> - bool array_spacing_lod0;
> >> + bool non_mip_arrays;
> >>
> >> /**
> >> * Format that should be used when setting up the surface state for
> >> this
> >> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> index 76044b2..9e2720b 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> @@ -241,7 +241,7 @@ brw_miptree_layout_texture_array(struct brw_context
> >> *brw,
> >>
> >> h0 = ALIGN(mt->physical_height0, mt->align_h);
> >> h1 = ALIGN(minify(mt->physical_height0, 1), mt->align_h);
> >> - if (mt->array_spacing_lod0)
> >> + if (mt->non_mip_arrays)
> >> mt->qpitch = h0;
> >> else
> >> mt->qpitch = (h0 + h1 + (brw->gen >= 7 ? 12 : 11) * mt->align_h);
> >> diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> >> b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> >> index 0ad570b..c33cfeb 100644
> >> --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> >> +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
> >> @@ -169,7 +169,7 @@ gen7_blorp_emit_surface_state(struct brw_context *brw,
> >> if (surface->mt->align_w == 8)
> >> surf[0] |= GEN7_SURFACE_HALIGN_8;
> >>
> >> - if (surface->array_spacing_lod0)
> >> + if (surface->non_mip_arrays)
> >> surf[0] |= GEN7_SURFACE_ARYSPC_LOD0;
> >> else
> >> surf[0] |= GEN7_SURFACE_ARYSPC_FULL;
> >> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> >> b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> >> index 01120af..5d068d4 100644
> >> --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> >> +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> >> @@ -315,7 +315,7 @@ gen7_update_texture_surface(struct gl_context *ctx,
> >> uint32_t effective_depth = (tObj->Immutable && tObj->Target !=
> >> GL_TEXTURE_3D)
> >> ? tObj->NumLayers : mt->logical_depth0;
> >>
> >> - if (mt->array_spacing_lod0)
> >> + if (mt->non_mip_arrays)
> >> surf[0] |= GEN7_SURFACE_ARYSPC_LOD0;
> >>
> >> surf[1] = mt->bo->offset64 + mt->offset; /* reloc */
> >> @@ -508,8 +508,8 @@ gen7_update_renderbuffer_surface(struct brw_context
> >> *brw,
> >>
> >> surf[0] = surftype << BRW_SURFACE_TYPE_SHIFT |
> >> format << BRW_SURFACE_FORMAT_SHIFT |
> >> - (irb->mt->array_spacing_lod0 ? GEN7_SURFACE_ARYSPC_LOD0
> >> - : GEN7_SURFACE_ARYSPC_FULL) |
> >> + (irb->mt->non_mip_arrays ? GEN7_SURFACE_ARYSPC_LOD0
> >> + : GEN7_SURFACE_ARYSPC_FULL) |
> >> gen7_surface_tiling_mode(mt->tiling);
> >>
> >> if (irb->mt->align_h == 4)
> >> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> index b36ffc7..cc3b705 100644
> >> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> >> @@ -330,17 +330,17 @@ intel_miptree_create_layout(struct brw_context *brw,
> >> }
> >> }
> >>
> >> - /* array_spacing_lod0 is only used for non-IMS MSAA surfaces. TODO:
> >> can we
> >> + /* non_mip_arrays is only used for non-IMS MSAA surfaces. TODO: can we
> >> * use it elsewhere?
> >> */
> >> switch (mt->msaa_layout) {
> >> case INTEL_MSAA_LAYOUT_NONE:
> >> case INTEL_MSAA_LAYOUT_IMS:
> >> - mt->array_spacing_lod0 = false;
> >> + mt->non_mip_arrays = false;
> >> break;
> >> case INTEL_MSAA_LAYOUT_UMS:
> >> case INTEL_MSAA_LAYOUT_CMS:
> >> - mt->array_spacing_lod0 = true;
> >> + mt->non_mip_arrays = true;
> >> break;
> >> }
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> >> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> >> index bb04084..2d902c3 100644
> >> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> >> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> >> @@ -328,7 +328,7 @@ struct intel_mipmap_tree
> >> *
> >> * Corresponds to the surface_array_spacing bit in gen7_surface_state.
> >> */
> >> - bool array_spacing_lod0;
> >> + bool non_mip_arrays;
> >>
> >> /**
> >> * The distance in rows between array slices in an uncompressed
> >> surface.
> >> --
> >> 2.0.1
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
