<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Jan 21, 2017 at 11:12 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 Sat, Jan 21, 2017 at 11:05:52AM -0800, Jason Ekstrand wrote: > On Sat, Jan 21, 2017 at 1:19 AM, Pohjolainen, Topi > <[1]<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> 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]<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][3]<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? > > 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. </div></div>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. </blockquote></div> </div><div class="gmail_extra">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. :-) </div><div class="gmail_extra">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. </div></div>