[Mesa-dev] [PATCH 12/27] i965/gen6: Calculate stencil offset on demand
Pohjolainen, Topi
topi.pohjolainen at gmail.com
Sat Jan 21 19:12:48 UTC 2017
On Sat, Jan 21, 2017 at 11:05:52AM -0800, Jason Ekstrand wrote:
> On Sat, Jan 21, 2017 at 1:19 AM, Pohjolainen, Topi
> <[1]topi.pohjolainen at gmail.com> wrote:
>
> On Fri, Jan 20, 2017 at 04:35:23PM -0800, Jason Ekstrand wrote:
> > On Mon, Jan 16, 2017 at 1:13 AM, Topi Pohjolainen
> > <[1][2]topi.pohjolainen at gmail.com> wrote:
> >
> > This is kept on purpose in i965. It can be moved to ISL if it
> > is needed in vulkan.
> > Pointers to miptrees are given solely for verification
> purposes.
> > These will be dropped in following patches.
> > Signed-off-by: Topi Pohjolainen
> <[2][3]topi.pohjolainen at intel.com>
>
> > ---
> > src/mesa/drivers/dri/i965/brw_tex_layout.c | 65
> > +++++++++++++++++++++++++++
> > src/mesa/drivers/dri/i965/gen6_depth_state.c | 14 +++---
> > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 5 +++
> > 3 files changed, 78 insertions(+), 6 deletions(-)
> > diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > index 768f8a8..80b341a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > @@ -288,6 +288,71 @@ gen9_miptree_layout_1d(struct
> intel_mipmap_tree
> > *mt)
> > }
> > }
> > +static unsigned
> > +all_slices_at_each_lod_x_offset(unsigned w0, unsigned align,
> > unsigned level)
> > +{
> > + const unsigned w = level >= 2 ? minify(w0, 1) : 0;
> > + return ALIGN(w, align);
> > +}
> > +
> > +static unsigned
> > +all_slices_at_each_lod_y_offset(const struct isl_extent4d
> > *phys_level0_sa,
> > + enum isl_surf_dim dim, unsigned
> > align,
> > + unsigned level)
> > +{
> > + unsigned y = 0;
> > +
> > + /* Add vertical space taken by lower levels one by one.
> Levels
> > one and two
> > + * are side-by-side just below level zero. Levels three and
> > greater are
> > + * stacked one after another below level two.
> > + */
> > + for (unsigned i = 1; i <= level; ++i) {
> > + const unsigned d = dim == ISL_SURF_DIM_3D ?
> > + minify(phys_level0_sa->depth, i - 1) :
> > + phys_level0_sa->array_len;
> > +
> > + /* Levels two and greater are stacked just below level
> zero.
> > */
> > + if (i != 2) {
> > + const unsigned h = minify(phys_level0_sa->height, i -
> 1);
> > + y += d * ALIGN(h, align);
> > + }
> > + }
> > +
> > + return y;
> > +}
> > +
> > +uint32_t
> > +brw_stencil_all_slices_at_each_lod_offset(const struct isl_surf
> > *surf,
> > + const struct
> > intel_mipmap_tree *mt,
> > + unsigned level)
> > +{
> > + assert(mt->array_layout == ALL_SLICES_AT_EACH_LOD);
> > +
> > + const unsigned halign = 64;
> > + const unsigned valign = 64;
> > + const unsigned level_x = all_slices_at_each_lod_x_offset(
> > + surf->phys_level0_sa.width, halign, level);
> > + const unsigned level_y = all_slices_at_each_lod_y_offset(
> > + &surf->phys_level0_sa, surf->dim, valign, level);
> > +
> > + assert(level_x == mt->level[level].level_x);
> > + assert(level_y == mt->level[level].level_y);
> > +
> > + /* From Vol 2a, 11.5.6.2.1 3DSTATE_STENCIL_BUFFER, field
> > "Surface Pitch":
> > + * The pitch must be set to 2x the value computed based
> on
> > width, as
> > + * the stencil buffer is stored with two rows
> interleaved.
> > + *
> > + * While ISL surface stores the pitch expected by hardware,
> the
> > offset
> > + * into individual slices needs to be calculated as if rows
> are
> > + * interleaved.
> > + */
> > + const unsigned two_rows_interleaved_pitch = surf->row_pitch
> / 2;
> > +
> > + assert(two_rows_interleaved_pitch == mt->pitch);
> > +
> > + return level_y * two_rows_interleaved_pitch + level_x * 64;
> > +}
> >
> > There's really no good place in this series to make this comment
> but I
> > think this is as good as I'm going to get...
> > I can't quite tell what your long-term plan is for gen6 stencil
> and
> > HiZ. Eventually, I'd like to kill the ALL_SLICES_AT_EACH_LOD
> stuff.
> > The only reason why Jordan and the Igalia people did that in the
> first
> > place was so that they could re-use existing surface calculation
> code.
> > Really, the requirement is that gen6 doesn't support multiple
> miplevels
> > with HiZ and Stencil even though it does support arrays. The
> point of
> > the ALL_SLICES_AT_EACH_LOD layout is to, instead of making it an
> array
> > of miptrees, it lays it out into a bunch of single-LOD arrays.
> The
> > fact that they happen to be layed out like a miptree is
> immaterial.
> > When Nanley and I were talking about this in the office, he had a
> very
> > good suggestion. What he suggested was to just have an array of
> > isl_surf's, one for each miplevel and lay the single-LOD arrays
> out
> > linearly in memory. To compute the size of the gen6 stencil or
> HiZ
> > buffer you would do something like this;
> > uint32_t size = 0;
> > for (level = 0; level < num_levels; level++) {
> > isl_surf_init(&mt->gen6_per_level[level].surf,
> > .width = minify(width, level),
> > .height = minify(height, level),
> > .depth = 1,
> > .levels = 1,
> > .array_len = array_len,
> > .format = ISL_FORMAT_HIZ,
> > ...);
> > mt->gen6_per_level[level].offset = size;
> > size += mt->gen6_per_level[level].surf.size;
> > }
> > Then in the gen6 hiz and stencil setup code, you would just offset
> it
> > by mt->gen6_per_level[level].offset and be done with it. There's
> no
> > real reason why we need to keep all this ALL_SLICES_AT_EACH_LOD
> > calculation stuff indefinitely. We just need some way of putting
> all
> > of the per-LOD images in the same BO. (Technically, they don't
> even
> > need to go in the same BO but it's kind of nice to do it that
> way.)
> > Thoughts?
>
> This might be becoming a matter of taste debate. I'm probably
> thinking too
> much from "what does the hw setup need"-point of view. I'm thinking
> that
> as the hw really only wants offset and row pitch why do we need to
> maintain
> all the extra information. This is afterall something that can be
> calculated
> relatively straight forward if needs be. And the need is not clear -
> as hw
> setup don't need it, what is it for? Accessing individual slices
> using cpu
> is also just about the offset and shifting the base level
> dimensions.
> Also continuing with matter of taste. I'm not a big fan of storing
> information
> in tables if it is used infrequently and rather cheap to produce
> on-demand.
> (I'm seeing the array of isl_surf instances as a pre-computed lookup
> table).
>
> Honestly, I'm not married to the idea of storing all of the isl_surf's
> for each slice. We can store them or not. It was mostly about laying
> things out sequentially in memory rather than trying to place all of
> these single-LOD arrays in some sort of 2-D mipmap layout. Among other
> things, doing so wastes space compared to just storing the different
> LODs sequentially. Also, it makes for a bunch of painful calculations
> that we're doing for really no good reason. The calculation for the
> offset could be just a sum of the sizes of the previous miplevels.
I do like the idea of laying them out sequentially - like you said, current
layout doesn't serve anything, it just makes the calculations unnecessaryly
complex.
I can work on that change, but initially to me it seems safer on top of what I
have here. I'll look into it some more.
More information about the mesa-dev
mailing list