<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Jan 21, 2017 at 1:19 AM, Pohjolainen, Topi <<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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]<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> 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]<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> <div><div class="h5">> --- > 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? </div></div>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). </blockquote><div> </div><div>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. </div></div></div></div>