Re: [Mesa-dev] i965: Use isl for hiz and stencil
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 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
Re: [Mesa-dev] i965: Use isl for hiz and stencil
On Tue, May 9, 2017 at 9:43 AM, Pohjolainen, Topi < topi.pohjolai...@gmail.com> wrote: > On Tue, May 09, 2017 at 08:45:55AM -0700, Jason Ekstrand 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. > > Ah, right, my mistake. We need a little more than isl_surf instance though, > at least we need offset per level (unless we calculate that on-demand). > Of course we could use the current level/slice table but I'm hoping to get > rid > of that. > Yeah, something similar to anv_surface is probably needed. Though I would be inclined to call it brw_per_lod_surf or something more descriptive like that. > > > > > > > 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
Re: [Mesa-dev] i965: Use isl for hiz and stencil
On Tue, May 09, 2017 at 08:45:55AM -0700, Jason Ekstrand 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. Ah, right, my mistake. We need a little more than isl_surf instance though, at least we need offset per level (unless we calculate that on-demand). Of course we could use the current level/slice table but I'm hoping to get rid of that. > > > > 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
Re: [Mesa-dev] i965: Use isl for hiz and stencil
On Tue, May 09, 2017 at 08:48:08AM -0700, 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 can give the "array of ISL surfaces" approach a spin to see how it looks, I'm curious to see if few things become a little cleaner. > > On Tue, May 9, 2017 at 8:45 AM, Jason Ekstrand 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
Re: [Mesa-dev] i965: Use isl for hiz and stencil
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. On Tue, May 9, 2017 at 8:45 AM, Jason Ekstrand 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
Re: [Mesa-dev] i965: Use isl for hiz and stencil
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
Re: [Mesa-dev] i965: Use isl for hiz and stencil
On Mon, May 08, 2017 at 04:51:35PM -0700, Jason Ekstrand wrote: > On Wed, May 3, 2017 at 2:22 AM, Topi Pohjolainen > 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 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. 3) Instances in the array are not reference counted and therefore deleted simply by deallocating the malloced chunk underneath. 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? 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). 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? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] i965: Use isl for hiz and stencil
On Wed, May 3, 2017 at 2:22 AM, Topi Pohjolainen 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. > Patch 20 switches gen6 hiz to use back-to-back. > > Patches 22-37 prepare i965 driver to work with miptrees based on isl. > Patches 38 and 39 start to use isl for stencil surfaces and effectively > switches to back-to-back stencil layout on gen6. > Patch 25 is mostly unneeded but it doesn't hurt and it provides me the > tiling converter I need in patch 36. > > There are two uglies, patches 21 and 37. Perhaps Nanley, Jason or Chad > can help me with 21... > Other than making it "nicer" I don't think there's much you can do about 21. Arrayed HiZ is completely undocumented. Let's just do what works. For 37, that one strikes me as very odd as it seems to go against the hardware docs and you didn't give much of an explanation as to why it's needed. > Jason: You have reviewed most of 1-17, and I don't think they have >changed that much. > > Rafael: I have conflicting patches with your series addressing depth > and stencil state emission. We should try to land your patches > first and then I'll rebase this on top. > > > If we agree on the approach here, I'll continue with gen4/5 depth > surface alignment workaround aiming to base depth surfaces also on > isl. That should allow me to start using isl state emitter for > depth-hiz-stencil. > I've looked through the whole series and I think it seems reasonable. It lets us switch stuff over one bit at a time and then we can have one big patch at the end which deletes all of the "if (mt->surf.size > 0)" cases. --Jason > > CC: Jason Ekstrand > CC: Nanley Chery > CC: Chad Versace > CC: Rafael Antognolli > > Topi Pohjolainen (39): > i965/dbg: Add means for forcing stencil sampling using y-tiled copy > i965/gen6: Remove dead code in hiz surface setup > i965/blorp/gen6: Drop unnecessary stencil/hiz surf dimension adjust > i965/gen6: Calculate stencil offset on demand > i965/gen6: Calculate hiz offset on demand > i965/blorp/gen6: Use on-demand stencil/hiz offset resolvers > i965/gen6: Drop miptrees in depth/stencil offset resolvers > i965/blorp/gen6: Set aux pitch directly > i965/gen6/hiz: Add direct buffer size resolver > i965/gen6: Allocate hiz directly without miptree > i965/miptree: Refactor aux surface allocation > i965/miptree: Refactor ISL aux usage resolver > i965/miptree: Use ISL for MCS layouts > i965/miptree: Drop MIPTREE_LAYOUT_ACCELERATED_UPLOAD in mcs init > i965/miptree/gen7+: Use ISL for HIZ layouts > i965/blorp: Use hiz surface instead of creating copy > i965: Use stored hiz surface instead of creating copy > intel/isl/gen6: Add offsetting support for back-to-back layouts > intel/isl/gen6: Add size calculator for back-to-back layouts > i965/hiz/gen6: Use isl back-to-back layout > intel/isl/gen6/hack: Use hiz vertical alignment of 16 instead of 8 > i965/miptree: Add support for resolving offsets using isl > i965/blorp: Add support for isl based miptrees > i965: Prepare up/downsampling for isl based miptrees > i965: Prepare blit engine for isl based miptrees > i965: Prepare image validation for isl based miptrees > i965: Refactor miptree to isl converter and adjustment > i965: Prepare tex, img and rt state emission for isl based miptrees > i965: Prepare slice validator for isl based miptrees > i965: Prepare framebuffer validator for isl based miptrees > i965/tex: Prepare image update for isl based miptrees > i965: Prepare texture validator for isl based miptrees > i965: Prepare slice copy for isl based miptrees > i965/gen7: Prepare depth state emission for isl based miptrees > i965/gen8+: Prepare depth state emission for isl based miptrees > i965:
Re: [Mesa-dev] i965: Use isl for hiz and stencil
On Wed, May 03, 2017 at 05:07:32PM -0700, Rafael Antognolli wrote: > On Wed, May 03, 2017 at 12:22:13PM +0300, Topi Pohjolainen 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. > > > > Patch 20 switches gen6 hiz to use back-to-back. > > > > Patches 22-37 prepare i965 driver to work with miptrees based on isl. > > Patches 38 and 39 start to use isl for stencil surfaces and effectively > > switches to back-to-back stencil layout on gen6. > > Patch 25 is mostly unneeded but it doesn't hurt and it provides me the > > tiling converter I need in patch 36. > > > > There are two uglies, patches 21 and 37. Perhaps Nanley, Jason or Chad > > can help me with 21... > > > > Jason: You have reviewed most of 1-17, and I don't think they have > >changed that much. > > > > Rafael: I have conflicting patches with your series addressing depth > > and stencil state emission. We should try to land your patches > > first and then I'll rebase this on top. > > Hey Topi, thanks for letting me know. Yeah, I saw the conflicts you > mention, and there are also conflicts with depth buffer state, which I'm > also converting to genxml. But from what I looked, it doesn't seem like > it's going to be too hard to rebase/solve the conflicts. Actually there wasn't conflict yet with the series you had in the list. You are planning to work on depth/stencil buffer state? There is support in ISL for that which I'm planning to use eventually. > > > If we agree on the approach here, I'll continue with gen4/5 depth > > surface alignment workaround aiming to base depth surfaces also on > > isl. That should allow me to start using isl state emitter for > > depth-hiz-stencil. > > > > CC: Jason Ekstrand > > CC: Nanley Chery > > CC: Chad Versace > > CC: Rafael Antognolli > > > > Topi Pohjolainen (39): > > i965/dbg: Add means for forcing stencil sampling using y-tiled copy > > i965/gen6: Remove dead code in hiz surface setup > > i965/blorp/gen6: Drop unnecessary stencil/hiz surf dimension adjust > > i965/gen6: Calculate stencil offset on demand > > i965/gen6: Calculate hiz offset on demand > > i965/blorp/gen6: Use on-demand stencil/hiz offset resolvers > > i965/gen6: Drop miptrees in depth/stencil offset resolvers > > i965/blorp/gen6: Set aux pitch directly > > i965/gen6/hiz: Add direct buffer size resolver > > i965/gen6: Allocate hiz directly without miptree > > i965/miptree: Refactor aux surface allocation > > i965/miptree: Refactor ISL aux usage resolver > > i965/miptree: Use ISL for MCS layouts > > i965/miptree: Drop MIPTREE_LAYOUT_ACCELERATED_UPLOAD in mcs init > > i965/miptree/gen7+: Use ISL for HIZ layouts > > i965/blorp: Use hiz surface instead of creating copy > > i965: Use stored hiz surface instead of creating copy > > intel/isl/gen6: Add offsetting support for back-to-back layouts > > intel/isl/gen6: Add size calculator for back-to-back layouts > > i965/hiz/gen6: Use isl back-to-back layout > > intel/isl/gen6/hack: Use hiz vertical alignment of 16 instead of 8 > > i965/miptree: Add support for resolving offsets using isl > > i965/blorp: Add support for isl based miptrees > > i965: Prepare up/downsampling for isl based miptrees > > i965: Prepare blit engine for isl based miptrees > > i965: Prepare image validation for isl based miptrees > > i965: Refactor miptree to isl converter and adjustment > > i965: Prepare tex, img and rt state emission for isl based miptrees > > i965: Prepare slice validator for isl based miptrees > > i965: Prepare framebuffer validator for isl based miptrees > > i965/tex: Prepare image update for isl based miptrees > > i965: Prepare texture validator for isl based miptrees > > i965: Prepare slice copy for isl based miptrees > > i965/gen7: Prepare depth state emission for isl based miptrees > > i965/gen8+: Prepare depth state emission for isl based miptrees > > i965: Add isl based miptree creator > > intel/isl/gen7/hack: Use stencil vertical alignment of 8 instead of 4 > > i965/miptree: Represent w-tiled stencil surfaces with isl > > i965/miptree: Represent y-tiled stencil copies with isl > > > > src/intel/blorp/blorp.c | 4 +- > > src/intel/blorp/blorp_blit.c |
Re: [Mesa-dev] i965: Use isl for hiz and stencil
On Wed, May 03, 2017 at 12:22:13PM +0300, Topi Pohjolainen 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. > > Patch 20 switches gen6 hiz to use back-to-back. > > Patches 22-37 prepare i965 driver to work with miptrees based on isl. > Patches 38 and 39 start to use isl for stencil surfaces and effectively > switches to back-to-back stencil layout on gen6. > Patch 25 is mostly unneeded but it doesn't hurt and it provides me the > tiling converter I need in patch 36. > > There are two uglies, patches 21 and 37. Perhaps Nanley, Jason or Chad > can help me with 21... > > Jason: You have reviewed most of 1-17, and I don't think they have >changed that much. > > Rafael: I have conflicting patches with your series addressing depth > and stencil state emission. We should try to land your patches > first and then I'll rebase this on top. Hey Topi, thanks for letting me know. Yeah, I saw the conflicts you mention, and there are also conflicts with depth buffer state, which I'm also converting to genxml. But from what I looked, it doesn't seem like it's going to be too hard to rebase/solve the conflicts. > If we agree on the approach here, I'll continue with gen4/5 depth > surface alignment workaround aiming to base depth surfaces also on > isl. That should allow me to start using isl state emitter for > depth-hiz-stencil. > > CC: Jason Ekstrand > CC: Nanley Chery > CC: Chad Versace > CC: Rafael Antognolli > > Topi Pohjolainen (39): > i965/dbg: Add means for forcing stencil sampling using y-tiled copy > i965/gen6: Remove dead code in hiz surface setup > i965/blorp/gen6: Drop unnecessary stencil/hiz surf dimension adjust > i965/gen6: Calculate stencil offset on demand > i965/gen6: Calculate hiz offset on demand > i965/blorp/gen6: Use on-demand stencil/hiz offset resolvers > i965/gen6: Drop miptrees in depth/stencil offset resolvers > i965/blorp/gen6: Set aux pitch directly > i965/gen6/hiz: Add direct buffer size resolver > i965/gen6: Allocate hiz directly without miptree > i965/miptree: Refactor aux surface allocation > i965/miptree: Refactor ISL aux usage resolver > i965/miptree: Use ISL for MCS layouts > i965/miptree: Drop MIPTREE_LAYOUT_ACCELERATED_UPLOAD in mcs init > i965/miptree/gen7+: Use ISL for HIZ layouts > i965/blorp: Use hiz surface instead of creating copy > i965: Use stored hiz surface instead of creating copy > intel/isl/gen6: Add offsetting support for back-to-back layouts > intel/isl/gen6: Add size calculator for back-to-back layouts > i965/hiz/gen6: Use isl back-to-back layout > intel/isl/gen6/hack: Use hiz vertical alignment of 16 instead of 8 > i965/miptree: Add support for resolving offsets using isl > i965/blorp: Add support for isl based miptrees > i965: Prepare up/downsampling for isl based miptrees > i965: Prepare blit engine for isl based miptrees > i965: Prepare image validation for isl based miptrees > i965: Refactor miptree to isl converter and adjustment > i965: Prepare tex, img and rt state emission for isl based miptrees > i965: Prepare slice validator for isl based miptrees > i965: Prepare framebuffer validator for isl based miptrees > i965/tex: Prepare image update for isl based miptrees > i965: Prepare texture validator for isl based miptrees > i965: Prepare slice copy for isl based miptrees > i965/gen7: Prepare depth state emission for isl based miptrees > i965/gen8+: Prepare depth state emission for isl based miptrees > i965: Add isl based miptree creator > intel/isl/gen7/hack: Use stencil vertical alignment of 8 instead of 4 > i965/miptree: Represent w-tiled stencil surfaces with isl > i965/miptree: Represent y-tiled stencil copies with isl > > src/intel/blorp/blorp.c | 4 +- > src/intel/blorp/blorp_blit.c | 11 +- > src/intel/common/gen_debug.c | 1 + > src/intel/common/gen_debug.h | 1 + > src/intel/isl/isl.c | 55 +- > src/intel/isl/isl.h | 20 +- > src/intel/isl/isl_gen6.c | 109 +++ > src/intel/isl/isl_gen7.c | 6 +- > src/intel/isl/isl_storage_image.c| 3 +- > src/mesa/driv
[Mesa-dev] i965: Use isl for hiz and stencil
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. Patch 20 switches gen6 hiz to use back-to-back. Patches 22-37 prepare i965 driver to work with miptrees based on isl. Patches 38 and 39 start to use isl for stencil surfaces and effectively switches to back-to-back stencil layout on gen6. Patch 25 is mostly unneeded but it doesn't hurt and it provides me the tiling converter I need in patch 36. There are two uglies, patches 21 and 37. Perhaps Nanley, Jason or Chad can help me with 21... Jason: You have reviewed most of 1-17, and I don't think they have changed that much. Rafael: I have conflicting patches with your series addressing depth and stencil state emission. We should try to land your patches first and then I'll rebase this on top. If we agree on the approach here, I'll continue with gen4/5 depth surface alignment workaround aiming to base depth surfaces also on isl. That should allow me to start using isl state emitter for depth-hiz-stencil. CC: Jason Ekstrand CC: Nanley Chery CC: Chad Versace CC: Rafael Antognolli Topi Pohjolainen (39): i965/dbg: Add means for forcing stencil sampling using y-tiled copy i965/gen6: Remove dead code in hiz surface setup i965/blorp/gen6: Drop unnecessary stencil/hiz surf dimension adjust i965/gen6: Calculate stencil offset on demand i965/gen6: Calculate hiz offset on demand i965/blorp/gen6: Use on-demand stencil/hiz offset resolvers i965/gen6: Drop miptrees in depth/stencil offset resolvers i965/blorp/gen6: Set aux pitch directly i965/gen6/hiz: Add direct buffer size resolver i965/gen6: Allocate hiz directly without miptree i965/miptree: Refactor aux surface allocation i965/miptree: Refactor ISL aux usage resolver i965/miptree: Use ISL for MCS layouts i965/miptree: Drop MIPTREE_LAYOUT_ACCELERATED_UPLOAD in mcs init i965/miptree/gen7+: Use ISL for HIZ layouts i965/blorp: Use hiz surface instead of creating copy i965: Use stored hiz surface instead of creating copy intel/isl/gen6: Add offsetting support for back-to-back layouts intel/isl/gen6: Add size calculator for back-to-back layouts i965/hiz/gen6: Use isl back-to-back layout intel/isl/gen6/hack: Use hiz vertical alignment of 16 instead of 8 i965/miptree: Add support for resolving offsets using isl i965/blorp: Add support for isl based miptrees i965: Prepare up/downsampling for isl based miptrees i965: Prepare blit engine for isl based miptrees i965: Prepare image validation for isl based miptrees i965: Refactor miptree to isl converter and adjustment i965: Prepare tex, img and rt state emission for isl based miptrees i965: Prepare slice validator for isl based miptrees i965: Prepare framebuffer validator for isl based miptrees i965/tex: Prepare image update for isl based miptrees i965: Prepare texture validator for isl based miptrees i965: Prepare slice copy for isl based miptrees i965/gen7: Prepare depth state emission for isl based miptrees i965/gen8+: Prepare depth state emission for isl based miptrees i965: Add isl based miptree creator intel/isl/gen7/hack: Use stencil vertical alignment of 8 instead of 4 i965/miptree: Represent w-tiled stencil surfaces with isl i965/miptree: Represent y-tiled stencil copies with isl src/intel/blorp/blorp.c | 4 +- src/intel/blorp/blorp_blit.c | 11 +- src/intel/common/gen_debug.c | 1 + src/intel/common/gen_debug.h | 1 + src/intel/isl/isl.c | 55 +- src/intel/isl/isl.h | 20 +- src/intel/isl/isl_gen6.c | 109 +++ src/intel/isl/isl_gen7.c | 6 +- src/intel/isl/isl_storage_image.c| 3 +- src/mesa/drivers/dri/i965/brw_blorp.c| 124 ++-- src/mesa/drivers/dri/i965/brw_misc_state.c | 16 +- src/mesa/drivers/dri/i965/brw_tex_layout.c | 16 + src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 114 ++-- src/mesa/drivers/dri/i965/gen6_depth_state.c | 50 +- src/mesa/drivers/dri/i965/gen7_misc_state.c | 31 +- src/mesa/drivers/dri/i965/gen8_depth_state.c | 47 +- src/mesa/drivers/dri/i965/intel_blit.c | 45 +- src/mesa/drivers/dri/i965/intel_blit.h | 13 +