Bug 109985 - [SNB] INTEL_DEBUG=nohiz is likely broken for depth/stencil buffers
Summary: [SNB] INTEL_DEBUG=nohiz is likely broken for depth/stencil buffers
Status: NEEDINFO
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Intel 3D Bugs Mailing List
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-12 23:15 UTC by Nanley Chery
Modified: 2019-06-25 07:51 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
SNB nohiz piglit failures (37.65 KB, text/plain)
2019-03-14 20:39 UTC, Nanley Chery
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nanley Chery 2019-03-12 23:15:05 UTC
For SNB, INTEL_DEBUG=nohiz seems to be broken in two places:

1. In miptree_create() in intel_mipmap_tree.c. Trying to create a depth/stencil buffer will cause a depth miptree to be created without stencil.

2. In isl_genX(emit_depth_stencil_hiz_s) in isl_emit_depth_stencil.c. Emitting a depth/stencil buffer instruction will unconditionally force HiZ on.

I haven't tested/verified these observations.

mesa version: ae77f1236862e73c1ac250898924c648d481bda4
Comment 1 Denis 2019-03-13 09:31:22 UTC
hi Nanley, do you have any test which could help to reproduce this? That would be very helpful
Comment 2 Nanley Chery 2019-03-13 18:42:41 UTC
(In reply to Denis from comment #1)
> hi Nanley, do you have any test which could help to reproduce this? That
> would be very helpful

I don't have any specific test in mind. However, I think there's a high chance of finding one by running the piglit test suite on SNB with the debug flag enabled.
Comment 3 Lionel Landwerlin 2019-03-13 19:13:50 UTC
(In reply to Nanley Chery from comment #2)
> (In reply to Denis from comment #1)
> > hi Nanley, do you have any test which could help to reproduce this? That
> > would be very helpful
> 
> I don't have any specific test in mind. However, I think there's a high
> chance of finding one by running the piglit test suite on SNB with the debug
> flag enabled.

Maybe ./bin/hiz ?
Comment 4 Denis 2019-03-14 08:13:08 UTC
Hm, thanks for suggesting. Will check today
Comment 5 Denis 2019-03-14 17:19:05 UTC
hi again. If Nanley's suspicion is correct, then it couldn't be triggered by piglit tests. I made two parallel runs with/without flag, below you may see all information (two last tests are running toooo long, that's why I didn't wait for the last one. Tomorrow will provide run results with detailed information)
upd - just thought that I provided same folders for test results :( So I think there will be only results from second run, without flag. If needs, tomorrow I will make one more try.


1. Compiled mesa from git master
meson -Dbuildtype=debug -Dvalgrind=false -Ddri-drivers=i965 -Dgallium-drivers= -Dvulkan-drivers= -Dgallium-omx="disabled" -Dplatforms=x11,drm,surfaceless -Dtools=intel ./mbuild64/

2. Ran piglit tests in two cmd's. Below you may see results. They were the same.
HW:
OpenGL renderer string: Mesa DRI Intel(R) Sandybridge Mobile 
OpenGL core profile version string: 3.3 (Core Profile) Mesa 18.3.4 (git-b26488dead)
4.18.16-041816-generic
ubuntu 18.10
_______________________________
with INTEL_DEBUG=nohiz

ubuntu@ubuntu-HP-ProBook-6360b:~/repository/piglit$ ./piglit run all ~/repository/piglit/all/
wflinfo utility not found.pass: 3786, warn: 3, fail: 200, crash: 3 \||-
wflinfo utility not found.
wflinfo utility not found.
wflinfo utility not found.
wflinfo utility not found.
wflinfo utility not found.
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|\-
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|\\
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|\/
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|\ 
wflinfo utility not found.
wflinfo utility not found.
[54959/54960] skip: 50654, pass: 4099, warn: 3, fail: 200, crash: 3 \|\


without:

ubuntu@ubuntu-HP-ProBook-6360b:~/repository/piglit$ ./piglit run all ~/repository/piglit/all/
wflinfo utility not found.pass: 3786, warn: 3, fail: 200, crash: 3 \||-
wflinfo utility not found.
wflinfo utility not found.
wflinfo utility not found.
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|-\
wflinfo utility not found.
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|--
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|-\
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|-/
wflinfo utility not found.pass: 3787, warn: 3, fail: 200, crash: 3 \|\ 
wflinfo utility not found.
wflinfo utility not found.
[54959/54960] skip: 50654, pass: 4099, warn: 3, fail: 200, crash: 3 \|\
Comment 6 Denis 2019-03-14 17:19:49 UTC
btw - first of all I ran ./bin/hiz test. It passed successfully in both cases
Comment 7 Nanley Chery 2019-03-14 20:38:06 UTC
(In reply to Denis from comment #6)
> btw - first of all I ran ./bin/hiz test. It passed successfully in both cases

I wonder why you aren't seeing any failures. I just tested this configuration in our CI and am seeing over 400 failures in piglit. The output of ./bin/hiz was this error:

hiz: ../src/intel/isl/isl.c:1921: isl_emit_depth_stencil_hiz_s: Assertion `(info->stencil_surf->usage & ISL_SURF_USAGE_STENCIL_BIT)' failed.

Some of the failing tests include:
    piglit.spec.!opengl 1_0.gl-1_0-drawpixels-depth-test.snbm64
    piglit.spec.!opengl 1_0.gl-1_0-drawpixels-stencil-test.snbm64
    piglit.spec.!opengl 1_0.gl-1_0-no-op-paths.snbm64
    piglit.spec.!opengl 1_0.gl-1_0-readpixsanity.snbm64
    piglit.spec.!opengl 1_0.gl-1_0-scissor-depth-clear-negative-xy.snbm64
    piglit.spec.!opengl 1_0.gl-1_0-scissor-depth-clear.snbm64
    piglit.spec.!opengl 1_0.gl-1_0-scissor-many.snbm64
    piglit.spec.!opengl 1_0.gl-1_0-scissor-stencil-clear.snbm64
    piglit.spec.!opengl 1_1.copyteximage 1d.snbm64
    piglit.spec.!opengl 1_1.copyteximage 2d.snbm64
    piglit.spec.!opengl 1_1.depthfunc.snbm64
    piglit.spec.!opengl 1_1.depthrange-clear.snbm64
    piglit.spec.!opengl 1_1.depthstencil-default_fb-blit.snbm64
    piglit.spec.!opengl 1_1.depthstencil-default_fb-clear samples=2.snbm64
    piglit.spec.!opengl 1_1.depthstencil-default_fb-clear.snbm64
    piglit.spec.!opengl 1_1.depthstencil-default_fb-copypixels.snbm64
    piglit.spec.!opengl 1_1.depthstencil-default_fb-readpixels-24_8.snbm64
    piglit.spec.!opengl 1_1.depthstencil-default_fb-readpixels-32f_24_8_rev.snbm64
    piglit.spec.!opengl 1_1.depthstencil-default_fb-readpixels-float-and-ushort.snbm64
    piglit.spec.!opengl 1_1.draw-pixels.snbm64
    piglit.spec.!opengl 1_1.drawpix-z.snbm64
    piglit.spec.!opengl 1_1.getteximage-depth.snbm64
    piglit.spec.!opengl 1_1.hiz.snbm64
    piglit.spec.!opengl 1_1.polygon-mode-offset.snbm64
    piglit.spec.!opengl 1_1.polygon-offset.snbm64
    piglit.spec.!opengl 1_1.readpix-z.snbm64
    piglit.spec.!opengl 1_1.stencil-drawpixels.snbm64
    piglit.spec.!opengl 1_1.texsubimage-depth-formats.snbm64
    piglit.spec.!opengl 1_2.copyteximage 3d.snbm64
    piglit.spec.!opengl 1_4.copy-pixels.snbm64
    piglit.spec.!opengl 1_4.gl-1_4-polygon-offset.snbm64
    piglit.spec.!opengl 1_4.stencil-wrap.snbm64
    piglit.spec.!opengl 1_5.normal3b3s-invariance-byte.snbm64
    piglit.spec.!opengl 1_5.normal3b3s-invariance-short.snbm64
    piglit.spec.!opengl 2_0.clear-varray-2_0.snbm64
    piglit.spec.!opengl 2_0.early-z.snbm64
    piglit.spec.!opengl 2_0.gl-2_0-two-sided-stencil.snbm64
    piglit.spec.!opengl 2_0.stencil-twoside.snbm64
    piglit.spec.!opengl 3_0.clearbuffer-depth-stencil.snbm64
    piglit.spec.!opengl 3_0.clearbuffer-depth.snbm64
    piglit.spec.!opengl 3_0.clearbuffer-stencil.snbm64
    piglit.spec.!opengl 3_2.clear-no-buffers.snbm64

I'll attach a file with the rest.
Comment 8 Nanley Chery 2019-03-14 20:39:32 UTC
Created attachment 143669 [details]
SNB nohiz piglit failures
Comment 9 Denis 2019-03-15 08:03:07 UTC
Weird 8-/ ok, thank you for info, i have one more snb pc, will check and on it.
Comment 10 Denis 2019-03-15 08:06:00 UTC
meson -Dbuildtype=debug -Dvalgrind=false -Ddri-drivers=i965 -Dgallium-drivers= -Dvulkan-drivers= -Dgallium-omx="disabled" -Dplatforms=x11,drm,surfaceless -Dtools=intel ./mbuild64/


Does this enough to enable debugging with meson.? Or i missed some flags?
Comment 11 Denis 2019-03-15 11:01:03 UTC
my bad, sorry. There was a mistake in the export variable :(
I reproduced this issue, got same assertion for ./hiz test. Investigating
Comment 12 asimiklit 2019-03-29 15:39:42 UTC
(In reply to Nanley Chery from comment #0)
> For SNB, INTEL_DEBUG=nohiz seems to be broken in two places:
> 
> 1. In miptree_create() in intel_mipmap_tree.c. Trying to create a
> depth/stencil buffer will cause a depth miptree to be created without
> stencil.

As far as found this code disallows us to create combined depth/stencil on SNB:
   mesa_format mt_fmt = format;
   if (!_mesa_is_format_color_format(format) && devinfo->gen >= 6) {
      /* Fix up the Z miptree format for how we're splitting out separate
       * stencil. Gen7 expects there to be no stencil bits in its depth buffer.
       */
      mt_fmt = intel_depth_format_for_depthstencil_format(format);
   }

   Note: This code just replaces MESA_FORMAT_Z24_UNORM_S8_UINT by MESA_FORMAT_Z24_UNORM_X8_UINT
         but in this case 'mt_surf_usage' does not return a ISL_SURF_USAGE_STENCIL_BIT and that leads to
         an assertion: assert(src_surf->surf->usage & ISL_SURF_USAGE_STENCIL_BIT); in a blorp_blit function.
         PS: Actually I didn't check if this assertion actually matter.

But just avoiding of this code doesn't make us happy too (
Because we fail a choosing of a surf tiling method and "isl_surf_choose_tiling" always returns false.

Now I am investigates a SNB PRM for the valid combinations of tiling methods with combined depth/stencil.

Any advices or suggestions are welcome :)

> 
> 2. In isl_genX(emit_depth_stencil_hiz_s) in isl_emit_depth_stencil.c.
> Emitting a depth/stencil buffer instruction will unconditionally force HiZ
> on.

It happens because we create a separate stencil here regardless "nohiz" flag
(under SNB we always jump into 'true' branch in the following code):

   if (screen->devinfo.has_hiz_and_separate_stencil) {
      rb = intel_create_private_renderbuffer(screen,
                                             MESA_FORMAT_Z24_UNORM_X8_UINT,
                                             num_samples);
      _mesa_attach_and_own_rb(fb, BUFFER_DEPTH, &rb->Base.Base);
      rb = intel_create_private_renderbuffer(screen, MESA_FORMAT_S_UINT8,
                                             num_samples);
      _mesa_attach_and_own_rb(fb, BUFFER_STENCIL, &rb->Base.Base);
   } else {
      /*
       * Use combined depth/stencil. Note that the renderbuffer is
       * attached to two attachment points.
       */
      rb = intel_create_private_renderbuffer(screen,
                                             MESA_FORMAT_Z24_UNORM_S8_UINT,
                                             num_samples);
      _mesa_attach_and_own_rb(fb, BUFFER_DEPTH, &rb->Base.Base);
      _mesa_attach_and_reference_rb(fb, BUFFER_STENCIL, &rb->Base.Base);
   }

A separate stencil forces us to emit a "Hierarchical Depth Buffer Enable" as a TRUE because of:
   SNB PRM Vol 2 Part 1 about field 'Separate Stencil Buffer Enable':
   [DevSNB]: This field must be set to the same value (enabled or disabled) as Hierarchical Depth Buffer Enable

So looks like we need to create a combined depth/stencil there for has_separate_stencil = false
case but as you mentioned above we unable to create combined depth/stencil due to issue 1.

> 
> I haven't tested/verified these observations.
> 
> mesa version: ae77f1236862e73c1ac250898924c648d481bda4
Comment 13 Nanley Chery 2019-03-29 23:53:45 UTC
(In reply to asimiklit from comment #12)
> (In reply to Nanley Chery from comment #0)
> > For SNB, INTEL_DEBUG=nohiz seems to be broken in two places:
> > 
> > 1. In miptree_create() in intel_mipmap_tree.c. Trying to create a
> > depth/stencil buffer will cause a depth miptree to be created without
> > stencil.
> 
> As far as found this code disallows us to create combined depth/stencil on
> SNB:
>    mesa_format mt_fmt = format;
>    if (!_mesa_is_format_color_format(format) && devinfo->gen >= 6) {
>       /* Fix up the Z miptree format for how we're splitting out separate
>        * stencil. Gen7 expects there to be no stencil bits in its depth
> buffer.
>        */
>       mt_fmt = intel_depth_format_for_depthstencil_format(format);
>    }
> 
>    Note: This code just replaces MESA_FORMAT_Z24_UNORM_S8_UINT by
> MESA_FORMAT_Z24_UNORM_X8_UINT
>          but in this case 'mt_surf_usage' does not return a
> ISL_SURF_USAGE_STENCIL_BIT and that leads to
>          an assertion: assert(src_surf->surf->usage &
> ISL_SURF_USAGE_STENCIL_BIT); in a blorp_blit function.
>          PS: Actually I didn't check if this assertion actually matter.
> 
> But just avoiding of this code doesn't make us happy too (
> Because we fail a choosing of a surf tiling method and
> "isl_surf_choose_tiling" always returns false.
> 
> Now I am investigates a SNB PRM for the valid combinations of tiling methods
> with combined depth/stencil.
> 
> Any advices or suggestions are welcome :)
> 

The cause of this problem is that we have two non-equivalent conditions for selecting the main miptree format and creating a separate stencil miptree: The condition you mentioned above and the call to needs_separate_stencil(). One solution is to refactor and reuse needs_separate_stencil() for the format selection.

> > 
> > 2. In isl_genX(emit_depth_stencil_hiz_s) in isl_emit_depth_stencil.c.
> > Emitting a depth/stencil buffer instruction will unconditionally force HiZ
> > on.
> 
> It happens because we create a separate stencil here regardless "nohiz" flag
> (under SNB we always jump into 'true' branch in the following code):
> 
>    if (screen->devinfo.has_hiz_and_separate_stencil) {
>       rb = intel_create_private_renderbuffer(screen,
>                                              MESA_FORMAT_Z24_UNORM_X8_UINT,
>                                              num_samples);
>       _mesa_attach_and_own_rb(fb, BUFFER_DEPTH, &rb->Base.Base);
>       rb = intel_create_private_renderbuffer(screen, MESA_FORMAT_S_UINT8,
>                                              num_samples);
>       _mesa_attach_and_own_rb(fb, BUFFER_STENCIL, &rb->Base.Base);
>    } else {
>       /*
>        * Use combined depth/stencil. Note that the renderbuffer is
>        * attached to two attachment points.
>        */
>       rb = intel_create_private_renderbuffer(screen,
>                                              MESA_FORMAT_Z24_UNORM_S8_UINT,
>                                              num_samples);
>       _mesa_attach_and_own_rb(fb, BUFFER_DEPTH, &rb->Base.Base);
>       _mesa_attach_and_reference_rb(fb, BUFFER_STENCIL, &rb->Base.Base);
>    }
> 
> A separate stencil forces us to emit a "Hierarchical Depth Buffer Enable" as
> a TRUE because of:
>    SNB PRM Vol 2 Part 1 about field 'Separate Stencil Buffer Enable':
>    [DevSNB]: This field must be set to the same value (enabled or disabled)
> as Hierarchical Depth Buffer Enable
> 

I see.

> So looks like we need to create a combined depth/stencil there for
> has_separate_stencil = false
> case but as you mentioned above we unable to create combined depth/stencil
> due to issue 1.
> 

I'm not very familiar with this section of code. My understanding of it is that we create object(s) to represent the depth-stencil portion of the default framebuffer here. Sometime later we create a miptree for each object. If that's true, we can get rid of the true branch. The miptree creation code will create a separate stencil buffer internally.
Comment 14 Nanley Chery 2019-04-08 21:14:21 UTC
(In reply to Nanley Chery from comment #13)
> (In reply to asimiklit from comment #12)
> > (In reply to Nanley Chery from comment #0)
> > > For SNB, INTEL_DEBUG=nohiz seems to be broken in two places:
> > > 
> > > 1. In miptree_create() in intel_mipmap_tree.c. Trying to create a
> > > depth/stencil buffer will cause a depth miptree to be created without
> > > stencil.
> > 
> > As far as found this code disallows us to create combined depth/stencil on
> > SNB:
> >    mesa_format mt_fmt = format;
> >    if (!_mesa_is_format_color_format(format) && devinfo->gen >= 6) {
> >       /* Fix up the Z miptree format for how we're splitting out separate
> >        * stencil. Gen7 expects there to be no stencil bits in its depth
> > buffer.
> >        */
> >       mt_fmt = intel_depth_format_for_depthstencil_format(format);
> >    }
> > 
> >    Note: This code just replaces MESA_FORMAT_Z24_UNORM_S8_UINT by
> > MESA_FORMAT_Z24_UNORM_X8_UINT
> >          but in this case 'mt_surf_usage' does not return a
> > ISL_SURF_USAGE_STENCIL_BIT and that leads to
> >          an assertion: assert(src_surf->surf->usage &
> > ISL_SURF_USAGE_STENCIL_BIT); in a blorp_blit function.
> >          PS: Actually I didn't check if this assertion actually matter.
> > 
> > But just avoiding of this code doesn't make us happy too (
> > Because we fail a choosing of a surf tiling method and
> > "isl_surf_choose_tiling" always returns false.
> > 
> > Now I am investigates a SNB PRM for the valid combinations of tiling methods
> > with combined depth/stencil.
> > 
> > Any advices or suggestions are welcome :)
> > 
> 
> The cause of this problem is that we have two non-equivalent conditions for
> selecting the main miptree format and creating a separate stencil miptree:
> The condition you mentioned above and the call to needs_separate_stencil().
> One solution is to refactor and reuse needs_separate_stencil() for the
> format selection.
> 

Sorry, I only addressed the problem with avoiding the format selection code.

The tiling issue you've pointed out is a bug in isl_gen6_filter_tiling(). For depth-stencil buffers on SNB, we have a Y-tiled surface with the elements laid out as described in the "3D Depth Buffer Surfaces" section of the PRM.
Comment 15 asimiklit 2019-04-09 09:00:41 UTC
(In reply to Nanley Chery from comment #14)
> (In reply to Nanley Chery from comment #13)
> > (In reply to asimiklit from comment #12)
> > > (In reply to Nanley Chery from comment #0)
> > > > For SNB, INTEL_DEBUG=nohiz seems to be broken in two places:
> > > > 
> > > > 1. In miptree_create() in intel_mipmap_tree.c. Trying to create a
> > > > depth/stencil buffer will cause a depth miptree to be created without
> > > > stencil.
> > > 
> > > As far as found this code disallows us to create combined depth/stencil on
> > > SNB:
> > >    mesa_format mt_fmt = format;
> > >    if (!_mesa_is_format_color_format(format) && devinfo->gen >= 6) {
> > >       /* Fix up the Z miptree format for how we're splitting out separate
> > >        * stencil. Gen7 expects there to be no stencil bits in its depth
> > > buffer.
> > >        */
> > >       mt_fmt = intel_depth_format_for_depthstencil_format(format);
> > >    }
> > > 
> > >    Note: This code just replaces MESA_FORMAT_Z24_UNORM_S8_UINT by
> > > MESA_FORMAT_Z24_UNORM_X8_UINT
> > >          but in this case 'mt_surf_usage' does not return a
> > > ISL_SURF_USAGE_STENCIL_BIT and that leads to
> > >          an assertion: assert(src_surf->surf->usage &
> > > ISL_SURF_USAGE_STENCIL_BIT); in a blorp_blit function.
> > >          PS: Actually I didn't check if this assertion actually matter.
> > > 
> > > But just avoiding of this code doesn't make us happy too (
> > > Because we fail a choosing of a surf tiling method and
> > > "isl_surf_choose_tiling" always returns false.
> > > 
> > > Now I am investigates a SNB PRM for the valid combinations of tiling methods
> > > with combined depth/stencil.
> > > 
> > > Any advices or suggestions are welcome :)
> > > 
> > 
> > The cause of this problem is that we have two non-equivalent conditions for
> > selecting the main miptree format and creating a separate stencil miptree:
> > The condition you mentioned above and the call to needs_separate_stencil().
> > One solution is to refactor and reuse needs_separate_stencil() for the
> > format selection.
> > 
> 
> Sorry, I only addressed the problem with avoiding the format selection code.
> 
> The tiling issue you've pointed out is a bug in isl_gen6_filter_tiling().
> For depth-stencil buffers on SNB, we have a Y-tiled surface with the
> elements laid out as described in the "3D Depth Buffer Surfaces" section of
> the PRM.

Thanks a lot for your help:)
At the moment I am working on a Iris issue, because it looks more actual today :),
I will return to this issue as far as complete the Iris issues :)

PS: I apologize for a delayed reply
Comment 16 asimiklit 2019-06-25 07:51:53 UTC
I have suggested the possible solution for this issue:
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1175


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.