On Tue 09 May 2017, Jason Ekstrand wrote: > All of the below being said, I'm fine with landing things with a > BACK_TO_BACK layout and then making the switch to one isl_surf per miplevel > later if that's an easier path.
I haven't read the series in full, but I agree with Jason here that an array of isl_surf, one per miplevel, makes more sense than a new isl layout. One reason is that's a more accurate description of the hardware's behavior. FWIW, before I left Intel, I had planned to implement gen6 hiz with an array of isl_surf. Don't let my comments block the series, though. I doubt I'll find the time to review it all. > On Tue, May 9, 2017 at 8:45 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote: > > > On Mon, May 8, 2017 at 11:10 PM, Pohjolainen, Topi < > > topi.pohjolai...@gmail.com> wrote: > > > >> On Mon, May 08, 2017 at 04:51:35PM -0700, Jason Ekstrand wrote: > >> > On Wed, May 3, 2017 at 2:22 AM, Topi Pohjolainen < > >> topi.pohjolai...@gmail.com > >> > > wrote: > >> > > >> > > Patches 1-17 are revision that > >> > > > >> > > - rework hiz on gen6 to use on-demand offset calculator allowing > >> > > one to drop dependency to miptree structure and > >> > > - rework all auxiliary surfaces to be created against isl directly. > >> > > > >> > > Patches 18 and 19 introduce new surface layout in ISL. This is called > >> > > back-to-back and similar to layout ALL_SLICES_AT_EACH_LOD found in > >> > > i965 for gen6 hiz and stencil. This layout stacks slices for each > >> level > >> > > after one and other, or back to back. All slices ate each lod is > >> almost > >> > > the same except that it places levels one and two side-by-side trying > >> > > to preserve space. Back-to-back wastes a little more memory but aligns > >> > > each level on page boundary simplifying driver logic. > >> > > > >> > > >> > My primary gripe here is that you seem to have half-added back-to-back > >> to > >> > ISL. If this layout is a long-term thing, then we should add a new > >> > ISL_DIM_LAYOUT_GEN6_BACK_TO_BACK layout and plumb your offset function > >> > through isl_surf_get_image_offset_sa. Is this intended to be a > >> permanent > >> > solution? I think eventually, I'd like us to go with one surf per > >> miplevel > >> > (which is almost the same) but I can see how this is easier at the > >> moment. > >> > However, I think this works sufficiently well that I'm ok with doing the > >> > back-to-back thing for a while. > >> > >> I thought about adding new layout type but couldn't decide which way is > >> better. It is easy to buy your arguments in favor, and I'm happy to give > >> it > >> a go. > >> If miptree per level is your number one choice, then lets go with that. > > > > > > I said "one surf per miplevel". I see no reason why we need N miptrees. > > > > > >> I just > >> need to check a few things first about the actual solution. I would see > >> something in these lines: > >> > >> 1) Add a dynamically allocated array of miptrees into miptree. This would > >> contain miptree instance per level. > >> > >> 2) Still uses one buffer object containing space for all levels. The > >> instances > >> in the array would either have their ::bo pointer zero or pointing to > >> the > >> parent ::bo. In both cases ::offset would point the start of the level. > >> > > > > Yes > > > > > >> 3) Instances in the array are not reference counted and therefore deleted > >> simply by deallocating the malloced chunk underneath. > >> > > > > If we have one isl_surf per miplevel and not a miptree per level, then I > > don't think this is an issue. > > > > > >> 4) Add similar dynamically allocated array of intel_miptree_aux_buffer > >> instances for hiz. Here also use one ::bo which would need to added to > >> miptree I think cause there ins't one in miptree. Or perhaps add the > >> array of aux buffers to aux buffer? > >> > > > > Looking at intel_miptree_aux_buffer, I think what we would end up with is > > an array of aux_buffers > > > > > >> 5) ISL doesn't need to know about this and hence we would add the total > >> space > >> calculator along with ::offset initialization in i965 (brw_tex_layout, > >> I think). > >> > > > > That's fine. We already do that in Vulkan with anv_surface. ::offset > > calculation can be done easily enough by just adding sizes. > > > > > >> 6) In Vulkan <-> GL interop, we'd pass single level arrays only as ISL > >> didn't > >> know about back-2-back. Or we simply don't care about gen6 as Vulkan > >> doesn't support it anyhow? > >> > > > > Yeah, we don't care about gen6. > > > > --Jason > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
