Re: [Mesa-dev] [PATCH 08/64] i965/blorp: Only set src_z for gen8+ 3D textures

[Chad Versace]

On Tue 21 Jun 2016, Pohjolainen, Topi wrote:
> On Mon, Jun 20, 2016 at 08:52:50PM -0700, Kenneth Graunke wrote:
> > On Friday, June 17, 2016 4:55:41 PM PDT Jason Ekstrand wrote:
> > > On Thu, Jun 16, 2016 at 10:08 AM, Chad Versace 
> > > wrote:
> > > 
> > > > On Sat 11 Jun 2016, Jason Ekstrand wrote:

> > > > Enlighten me. Why does blorp use 3D surfaces on gen >= 8 but not
> > > > earlier?


> > > History?  TBH, I'm not really sure.  Probably because SKL 3-D is different
> > > but you'd have to ask topi to be sure.
> > 
> > Historically, we did everything via tile x/y offsets, and so BLORP
> > worked that way too.  However, we never applied any offsets to the
> > auxiliary surfaces (i.e. CMS).  This seems fairly sketchy, and got
> > us into trouble when we ported it to Gen8.  As a hack, falling back
> > from CMS to UMS made things work out.
> > 
> > Rather than try and figure out proper offset calculations for CCS_D,
> > CCS_E, and HiZ, Topi decided to just access everything using level/layer
> > like we do for normal GL.  It seemed a lot safer, and worked out well.
> > 
> > Gen6 never uses CMS, so it works out.  Topi seemed to think that Gen7
> > can't do CMS for array textures (though I can't seem to find that code
> > to confirm), so it might work out.  I'm still not sure why Gen7 works.
> 
> I need to dig some more for this, I can't remember either.
> 
> > 
> > Honestly, I think it'd be better to convert Gen6-7 to be like Gen8+.

Yes, I agree. We should XOffset/YOffset when possible.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/64] i965/blorp: Only set src_z for gen8+ 3D textures

[Pohjolainen, Topi]

On Tue, Jun 21, 2016 at 03:51:15PM +0300, Pohjolainen, Topi wrote:
> On Mon, Jun 20, 2016 at 08:52:50PM -0700, Kenneth Graunke wrote:
> > On Friday, June 17, 2016 4:55:41 PM PDT Jason Ekstrand wrote:
> > > On Thu, Jun 16, 2016 at 10:08 AM, Chad Versace 
> > > wrote:
> > > 
> > > > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > > > Otherwise, we end up with a bogus value in the third component.  On
> > > > gen6-7
> > > > > where we always use 2D textures, this can cause problems if the
> > > > > SurfaceArray bit is set in the SURFACE_STATE.
> > > >
> > > > Enlighten me. Why does blorp use 3D surfaces on gen >= 8 but not
> > > > earlier?
> 
> Related (but not answering the question):
> on 9e4d19372b1d7f6ab12ddab1929a53335d9cce06:
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h 
> b/src/mesa/drivers/dri/i965/brw_blorp.h
> index 7ebcee2..a1aaa20 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp.h
> +++ b/src/mesa/drivers/dri/i965/brw_blorp.h
> @@ -196,8 +196,14 @@ struct brw_blorp_wm_push_constants
> float rect_grid_y1;
> brw_blorp_coord_transform_params x_transform;
> brw_blorp_coord_transform_params y_transform;
> +
> +   /* Minimum layer setting works for all the textures types but texture_3d
> +* for which the setting has no effect. Use the z-coordinate instead.
> +*/
> +   uint32_t src_z;
> +
> 
> The setting actually works on SKL+ if I remember correctly but as I needed
> something for gen8 I thought using the mechanism for both instead of adding
> yet another path.
> 
> > > >
> > > 
> > > History?  TBH, I'm not really sure.  Probably because SKL 3-D is different
> > > but you'd have to ask topi to be sure.
> > 
> > Historically, we did everything via tile x/y offsets, and so BLORP
> > worked that way too.  However, we never applied any offsets to the
> > auxiliary surfaces (i.e. CMS).  This seems fairly sketchy, and got
> > us into trouble when we ported it to Gen8.  As a hack, falling back
> > from CMS to UMS made things work out.
> > 
> > Rather than try and figure out proper offset calculations for CCS_D,
> > CCS_E, and HiZ, Topi decided to just access everything using level/layer
> > like we do for normal GL.  It seemed a lot safer, and worked out well.
> > 
> > Gen6 never uses CMS, so it works out.  Topi seemed to think that Gen7
> > can't do CMS for array textures (though I can't seem to find that code
> > to confirm), so it might work out.  I'm still not sure why Gen7 works.
> 
> I need to dig some more for this, I can't remember either.

I started wonder if we actually have arrayed compressed multisampled blits
broken an all gen7+. While gen8+ blorp uses proper hw support for texture
surfaces it still does the old manual offsetting for render surface (as
gen7+ blorp). And as Ken mentioned, there is no logic for offsetting the mcs
, and therefore the blits shouldn't work at all. We have plenty of tests for
msaa blits (color, stencil, depth) but I don't think any of them actually use
arrayed textures.

> 
> > 
> > Honestly, I think it'd be better to convert Gen6-7 to be like Gen8+.
> 
> 
> 
> > ___
> > 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/64] i965/blorp: Only set src_z for gen8+ 3D textures

[Pohjolainen, Topi]

On Mon, Jun 20, 2016 at 08:52:50PM -0700, Kenneth Graunke wrote:
> On Friday, June 17, 2016 4:55:41 PM PDT Jason Ekstrand wrote:
> > On Thu, Jun 16, 2016 at 10:08 AM, Chad Versace 
> > wrote:
> > 
> > > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > > Otherwise, we end up with a bogus value in the third component.  On
> > > gen6-7
> > > > where we always use 2D textures, this can cause problems if the
> > > > SurfaceArray bit is set in the SURFACE_STATE.
> > >
> > > Enlighten me. Why does blorp use 3D surfaces on gen >= 8 but not
> > > earlier?

Related (but not answering the question):
on 9e4d19372b1d7f6ab12ddab1929a53335d9cce06:

diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h 
b/src/mesa/drivers/dri/i965/brw_blorp.h
index 7ebcee2..a1aaa20 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.h
+++ b/src/mesa/drivers/dri/i965/brw_blorp.h
@@ -196,8 +196,14 @@ struct brw_blorp_wm_push_constants
float rect_grid_y1;
brw_blorp_coord_transform_params x_transform;
brw_blorp_coord_transform_params y_transform;
+
+   /* Minimum layer setting works for all the textures types but texture_3d
+* for which the setting has no effect. Use the z-coordinate instead.
+*/
+   uint32_t src_z;
+

The setting actually works on SKL+ if I remember correctly but as I needed
something for gen8 I thought using the mechanism for both instead of adding
yet another path.

> > >
> > 
> > History?  TBH, I'm not really sure.  Probably because SKL 3-D is different
> > but you'd have to ask topi to be sure.
> 
> Historically, we did everything via tile x/y offsets, and so BLORP
> worked that way too.  However, we never applied any offsets to the
> auxiliary surfaces (i.e. CMS).  This seems fairly sketchy, and got
> us into trouble when we ported it to Gen8.  As a hack, falling back
> from CMS to UMS made things work out.
> 
> Rather than try and figure out proper offset calculations for CCS_D,
> CCS_E, and HiZ, Topi decided to just access everything using level/layer
> like we do for normal GL.  It seemed a lot safer, and worked out well.
> 
> Gen6 never uses CMS, so it works out.  Topi seemed to think that Gen7
> can't do CMS for array textures (though I can't seem to find that code
> to confirm), so it might work out.  I'm still not sure why Gen7 works.

I need to dig some more for this, I can't remember either.

> 
> Honestly, I think it'd be better to convert Gen6-7 to be like Gen8+.



> ___
> 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] [PATCH 08/64] i965/blorp: Only set src_z for gen8+ 3D textures

[Kenneth Graunke]

On Friday, June 17, 2016 4:55:41 PM PDT Jason Ekstrand wrote:
> On Thu, Jun 16, 2016 at 10:08 AM, Chad Versace 
> wrote:
> 
> > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > Otherwise, we end up with a bogus value in the third component.  On
> > gen6-7
> > > where we always use 2D textures, this can cause problems if the
> > > SurfaceArray bit is set in the SURFACE_STATE.
> >
> > Enlighten me. Why does blorp use 3D surfaces on gen >= 8 but not
> > earlier?
> >
> 
> History?  TBH, I'm not really sure.  Probably because SKL 3-D is different
> but you'd have to ask topi to be sure.

Historically, we did everything via tile x/y offsets, and so BLORP
worked that way too.  However, we never applied any offsets to the
auxiliary surfaces (i.e. CMS).  This seems fairly sketchy, and got
us into trouble when we ported it to Gen8.  As a hack, falling back
from CMS to UMS made things work out.

Rather than try and figure out proper offset calculations for CCS_D,
CCS_E, and HiZ, Topi decided to just access everything using level/layer
like we do for normal GL.  It seemed a lot safer, and worked out well.

Gen6 never uses CMS, so it works out.  Topi seemed to think that Gen7
can't do CMS for array textures (though I can't seem to find that code
to confirm), so it might work out.  I'm still not sure why Gen7 works.

Honestly, I think it'd be better to convert Gen6-7 to be like Gen8+.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/64] i965/blorp: Only set src_z for gen8+ 3D textures

[Jason Ekstrand]

On Thu, Jun 16, 2016 at 10:08 AM, Chad Versace 
wrote:

> On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > Otherwise, we end up with a bogus value in the third component.  On
> gen6-7
> > where we always use 2D textures, this can cause problems if the
> > SurfaceArray bit is set in the SURFACE_STATE.
>
> Enlighten me. Why does blorp use 3D surfaces on gen >= 8 but not
> earlier?
>

History?  TBH, I'm not really sure.  Probably because SKL 3-D is different
but you'd have to ask topi to be sure.


>
> > ---
> >  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 11 +--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > index 782d285..cdb6b33 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > @@ -1846,8 +1846,15 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
> > brw_blorp_setup_coord_transform(_push_consts.y_transform,
> > src_y0, src_y1, dst_y0, dst_y1,
> mirror_y);
> >
> > -   params.wm_push_consts.src_z =
> > -  params.src.mt->target == GL_TEXTURE_3D ? params.src.layer : 0;
> > +   if (brw->gen >= 8 && params.src.mt->target == GL_TEXTURE_3D) {
> > +  /* On gen8+ we use actual 3-D textures so we need to pass the
> layer
> > +   * through to the sampler.
> > +   */
> > +  params.wm_push_consts.src_z = params.src.layer;
> > +   } else {
> > +  /* On gen7 and earlier, we fake everything with 2-D textures */
> > +  params.wm_push_consts.src_z = 0;
> > +   }
> >
> > if (params.dst.num_samples <= 1 && dst_mt->num_samples > 1) {
> >/* We must expand the rectangle we send through the rendering
> pipeline,
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/64] i965/blorp: Only set src_z for gen8+ 3D textures

[Chad Versace]

On Sat 11 Jun 2016, Jason Ekstrand wrote:
> Otherwise, we end up with a bogus value in the third component.  On gen6-7
> where we always use 2D textures, this can cause problems if the
> SurfaceArray bit is set in the SURFACE_STATE.

Enlighten me. Why does blorp use 3D surfaces on gen >= 8 but not
earlier?

> ---
>  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp 
> b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> index 782d285..cdb6b33 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> @@ -1846,8 +1846,15 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
> brw_blorp_setup_coord_transform(_push_consts.y_transform,
> src_y0, src_y1, dst_y0, dst_y1, mirror_y);
>  
> -   params.wm_push_consts.src_z =
> -  params.src.mt->target == GL_TEXTURE_3D ? params.src.layer : 0;
> +   if (brw->gen >= 8 && params.src.mt->target == GL_TEXTURE_3D) {
> +  /* On gen8+ we use actual 3-D textures so we need to pass the layer
> +   * through to the sampler.
> +   */
> +  params.wm_push_consts.src_z = params.src.layer;
> +   } else {
> +  /* On gen7 and earlier, we fake everything with 2-D textures */
> +  params.wm_push_consts.src_z = 0;
> +   }
>  
> if (params.dst.num_samples <= 1 && dst_mt->num_samples > 1) {
>/* We must expand the rectangle we send through the rendering pipeline,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 08/64] i965/blorp: Only set src_z for gen8+ 3D textures

[Jason Ekstrand]

Otherwise, we end up with a bogus value in the third component.  On gen6-7
where we always use 2D textures, this can cause problems if the
SurfaceArray bit is set in the SURFACE_STATE.
---
 src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp 
b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
index 782d285..cdb6b33 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
+++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
@@ -1846,8 +1846,15 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
brw_blorp_setup_coord_transform(_push_consts.y_transform,
src_y0, src_y1, dst_y0, dst_y1, mirror_y);
 
-   params.wm_push_consts.src_z =
-  params.src.mt->target == GL_TEXTURE_3D ? params.src.layer : 0;
+   if (brw->gen >= 8 && params.src.mt->target == GL_TEXTURE_3D) {
+  /* On gen8+ we use actual 3-D textures so we need to pass the layer
+   * through to the sampler.
+   */
+  params.wm_push_consts.src_z = params.src.layer;
+   } else {
+  /* On gen7 and earlier, we fake everything with 2-D textures */
+  params.wm_push_consts.src_z = 0;
+   }
 
if (params.dst.num_samples <= 1 && dst_mt->num_samples > 1) {
   /* We must expand the rectangle we send through the rendering pipeline,
-- 
2.5.0.400.gff86faf

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev