On Sat, Jan 21, 2017 at 1:19 AM, Pohjolainen, Topi <
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]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]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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
