Ben Widawsky <b...@bwidawsk.net> writes: > On Fri, Feb 20, 2015 at 10:31:08PM +0000, 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
