Re: [Mesa-dev] [PATCH 6/6] i965/skl: Don't use ALL_SLICES_AT_EACH_LOD

2015-03-10 Thread Neil Roberts 
Ben Widawsky b...@bwidawsk.net writes:

 On Fri, Feb 20, 2015 at 10:31:08PM +, Neil Roberts wrote:
 The render surface state command for Skylake doesn't have the surface
 array spacing bit so I don't think it's possible to select this
 layout. This avoids a kernel panic when running the piglit test below:

 Kernel panic!? Please, go on. We cannot cause kernel panics from
 userspace. It's a kernel bug if we do.

As far as I understand it's something like a GPU hang but then there's a
bug in the code to recover from the hang which makes the kernel crash.
I've talked to Damien about this and I think he was going to look at it
but it's a bit far down in his todo list. I was hoping that talking to
Damien is sufficient buck passing but perhaps it would be worth filing a
bug too.

 
 ext_framebuffer_multisample-no-color 8 stencil single
 
 However the test still fails so there may be something else wrong as
 well. The test was not causing a kernel panic before the patch to fix
 the qpitch.
 
 I think it's also not possible to select this layout on Gen8 so it may
 need to be changed to only be used on Gen7.

 I don't think this is the right answer. The array spacing bit goes
 away because we can manually specify the qpitch (I think).

 We should probably dig into this a bit more. I can help if you'd like.

I think we are only setting ALL_SLICES_AT_EACH_LOD in order to cause the
slices for the extra MSAA buffers to be tightly packed without space for
the mipmaps. This is not necessary with the new qpitch code on Skylake
because brw_miptree_layout_2d will set the total_height to not have
space for the mipmaps and the qpitch value is calculated based on that.
So in effect the slices are already automatically tightly packed
whenever mt-first_level==mt-last_level. I think we could make a
similar patch for Broadwell but I haven't looked into it.

The reason it breaks if ALL_SLICES_AT_EACH_LOD is chosen is because in
that case brw_miptree_layout_2d sets total_height to include all of the
slices. The qpitch is then chosen to include that total_height so it
ends up way too big and perhaps the hardware tries to read way off the
end of the buffer.

I guess it would be possible to change the qpitch code to specifically
handle ALL_SLICES_AT_EACH_LOD but I don't think this is the right
solution. It's better to just automatically pick the right qpitch value
when there are no mipmaps.

The trouble with ALL_SLICES_AT_EACH_LOD is that on Gen7 I think it is
possible to use that with mipmaps. In that case the slices for the
smaller levels would be placed after all of the slices for the larger
levels. This layout isn't possible to configure on Gen8+ so I think we
really want to kill this layout on those platforms. In practice this
doesn't currently matter on Gen8 because the only place where we choose
ALL_SLICES_AT_EACH_LOD is UMS and CMS MSAA layouts and in GL MSAA
textures currently can't have mipmaps (although one day maybe they
will?)

I think if you agree it would be good to go with this patch but maybe to
modify the commit message now that I understand it a bit better. Maybe
we should even add an assert in brw_miptree_layout to verify that
ALL_SLICES_AT_EACH_LOD is never used on Gen9 (and maybe Gen8+).

The Piglit test is actually now mysteriously passing although only with
this patch. I'm not exactly sure what has changed.

Regards,
- Neil


pgpg1gwtphoya.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 6/6] i965/skl: Don't use ALL_SLICES_AT_EACH_LOD

2015-03-09 Thread Ben Widawsky 
On Fri, Feb 20, 2015 at 10:31:08PM +, Neil Roberts wrote:
 The render surface state command for Skylake doesn't have the surface
 array spacing bit so I don't think it's possible to select this
 layout. This avoids a kernel panic when running the piglit test below:

Kernel panic!? Please, go on. We cannot cause kernel panics from userspace. It's
a kernel bug if we do.

 
 ext_framebuffer_multisample-no-color 8 stencil single
 
 However the test still fails so there may be something else wrong as
 well. The test was not causing a kernel panic before the patch to fix
 the qpitch.
 
 I think it's also not possible to select this layout on Gen8 so it may
 need to be changed to only be used on Gen7.

I don't think this is the right answer. The array spacing bit goes away because
we can manually specify the qpitch (I think).

We should probably dig into this a bit more. I can help if you'd like.

 ---
  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 26 --
  1 file changed, 16 insertions(+), 10 deletions(-)
 
 diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
 b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
 index 994670a..018e16b 100644
 --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
 +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
 @@ -371,19 +371,25 @@ intel_miptree_create_layout(struct brw_context *brw,
}
 }
  
 -   /* Set array_layout to ALL_SLICES_AT_EACH_LOD when gen7+ 
 array_spacing_lod0
 -* can be used. array_spacing_lod0 is only used for non-IMS MSAA surfaces.
 +   /* Set array_layout to ALL_SLICES_AT_EACH_LOD when array_spacing_lod0
 +* can be used. array_spacing_lod0 is only used for non-IMS MSAA surfaces
 +* on Gen 7 and 8.
  * TODO: can we use it elsewhere?
 +* TODO: does this actually work on Gen 8?
  */
 -   switch (mt-msaa_layout) {
 -   case INTEL_MSAA_LAYOUT_NONE:
 -   case INTEL_MSAA_LAYOUT_IMS:
 +   if (brw-gen = 9) {
mt-array_layout = ALL_LOD_IN_EACH_SLICE;
 -  break;
 -   case INTEL_MSAA_LAYOUT_UMS:
 -   case INTEL_MSAA_LAYOUT_CMS:
 -  mt-array_layout = ALL_SLICES_AT_EACH_LOD;
 -  break;
 +   } else {
 +  switch (mt-msaa_layout) {
 +  case INTEL_MSAA_LAYOUT_NONE:
 +  case INTEL_MSAA_LAYOUT_IMS:
 + mt-array_layout = ALL_LOD_IN_EACH_SLICE;
 + break;
 +  case INTEL_MSAA_LAYOUT_UMS:
 +  case INTEL_MSAA_LAYOUT_CMS:
 + mt-array_layout = ALL_SLICES_AT_EACH_LOD;
 + break;
 +  }
 }
  
 if (target == GL_TEXTURE_CUBE_MAP) {

-- 
Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 6/6] i965/skl: Don't use ALL_SLICES_AT_EACH_LOD

2015-02-20 Thread Neil Roberts 
The render surface state command for Skylake doesn't have the surface
array spacing bit so I don't think it's possible to select this
layout. This avoids a kernel panic when running the piglit test below:

ext_framebuffer_multisample-no-color 8 stencil single

However the test still fails so there may be something else wrong as
well. The test was not causing a kernel panic before the patch to fix
the qpitch.

I think it's also not possible to select this layout on Gen8 so it may
need to be changed to only be used on Gen7.
---
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 994670a..018e16b 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -371,19 +371,25 @@ intel_miptree_create_layout(struct brw_context *brw,
   }
}
 
-   /* Set array_layout to ALL_SLICES_AT_EACH_LOD when gen7+ array_spacing_lod0
-* can be used. array_spacing_lod0 is only used for non-IMS MSAA surfaces.
+   /* Set array_layout to ALL_SLICES_AT_EACH_LOD when array_spacing_lod0
+* can be used. array_spacing_lod0 is only used for non-IMS MSAA surfaces
+* on Gen 7 and 8.
 * TODO: can we use it elsewhere?
+* TODO: does this actually work on Gen 8?
 */
-   switch (mt-msaa_layout) {
-   case INTEL_MSAA_LAYOUT_NONE:
-   case INTEL_MSAA_LAYOUT_IMS:
+   if (brw-gen = 9) {
   mt-array_layout = ALL_LOD_IN_EACH_SLICE;
-  break;
-   case INTEL_MSAA_LAYOUT_UMS:
-   case INTEL_MSAA_LAYOUT_CMS:
-  mt-array_layout = ALL_SLICES_AT_EACH_LOD;
-  break;
+   } else {
+  switch (mt-msaa_layout) {
+  case INTEL_MSAA_LAYOUT_NONE:
+  case INTEL_MSAA_LAYOUT_IMS:
+ mt-array_layout = ALL_LOD_IN_EACH_SLICE;
+ break;
+  case INTEL_MSAA_LAYOUT_UMS:
+  case INTEL_MSAA_LAYOUT_CMS:
+ mt-array_layout = ALL_SLICES_AT_EACH_LOD;
+ break;
+  }
}
 
if (target == GL_TEXTURE_CUBE_MAP) {
-- 
1.9.3

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