Re: [Mesa-dev] i965: Use isl for hiz and stencil

[Chad Versace]

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

[Jason Ekstrand]

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

[Pohjolainen, Topi]

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

[Pohjolainen, Topi]

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

[Jason Ekstrand]

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

[Jason Ekstrand]

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

[Pohjolainen, Topi]

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

[Jason Ekstrand]

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
>   

Re: [Mesa-dev] i965: Use isl for hiz and stencil

[Pohjolainen, Topi]

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
> > 
> >  

Re: [Mesa-dev] i965: Use isl for hiz and stencil

[Rafael Antognolli]

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

[Mesa-dev] i965: Use isl for hiz and stencil

[Topi Pohjolainen]

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 +-