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

[Neil Roberts]

Ben Widawsky <ben at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 472 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150310/6dd05e50/attachment.sig>