On Sat, Jan 21, 2017 at 11:12 AM, Pohjolainen, Topi <
topi.pohjolai...@gmail.com> wrote:
> 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.pohjolai...@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.pohjolai...@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.pohjolai...@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.
>
That's perfectly reasonable. Let's just keep going and, now that we have
all the helpers, making that change won't be hard. I just wanted to make
sure we were on the same page before I started diving into detailed review
of gen6 miptree layout calculation code. :-)
One other note: This would me we would have a different pitch per LOD.
That's a bit annoying but probably not nearly as annoying as carying around
a bunch of 2D layout code we don't need.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
