Bug 50270 - [bisected] Fast depth clears break Unigine demos
Summary: [bisected] Fast depth clears break Unigine demos
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: high major
Assignee: Paul Berry
QA Contact:
Depends on:
Reported: 2012-05-23 01:57 UTC by libo
Modified: 2012-08-24 17:41 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

sanctuary rendering error pic (2.15 MB, image/png)
2012-05-23 01:57 UTC, libo

Description libo 2012-05-23 01:57:46 UTC
Created attachment 62005 [details]
sanctuary rendering error pic

System Environment:
Libdrm:     (master)2.4.33-20-gd72a44c7c4f5eea9c1e5bb0c36cb9e0224b9ca22
Mesa:       (8.0)fa68a8bae3961808288cfd84d5a7843f6fc0f317
Xserver:        (server-1.12-branch)xorg-server-1.12.1
Xf86_video_intel:  (master)2.19.0-36-ga3d37fb29f8dffb0e370ad95783994aaa7eccfaf
Cairo:      (master)aed94a5bc650f579368b4b814a8729570c32147e
Libva:      (master)aa8d1caa96eb2f83f4e24d7a7f400a675f6611d0
Kernel:     (drm-intel-fixes) 65e818660275ecda3702a4245f308923e3813a85
Os:     Fedora15

Bug detailed description:
There are rendering errors when run Unigine demos Sanctuary and Tropics.Pls see attached photo.

Reproduce steps:
3.export force_glsl_extensions_warn=true
4.sh 1920x1080_fullscreen.sh
Comment 1 Kenneth Graunke 2012-07-09 03:05:32 UTC
Bisected it:

68216f35814ab8d292f37b8c0fa0a5f181b7f20d is the first bad commit
commit 68216f35814ab8d292f37b8c0fa0a5f181b7f20d
Author: Eric Anholt <eric@anholt.net>
Date:   Thu May 17 22:03:32 2012 -0700

    i965/gen6+: Add support for fast depth clears.
    Improves citybench high-res performance 3.0% +- 0.4%, n=10.  Improves
    Lightsmark 1024x768 performance 0.74% +/- 0.20% (n=78).  No
    significant difference on openarena (n=5, didn't fast clear) or nexuiz
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
    Reviewed-by: Chad Versace <chad.versace@linux.intel.com>

:040000 040000 ad83e3c38a662757384501b6a7416a5b43129dbd e000ebb3ffdb0d4de1b702fbde49b4804ff98b99 M      src
Comment 2 Eric Anholt 2012-08-07 22:36:46 UTC
If I bail out of fast clear when depth_irb->mt_layer != 0, I don't see the artifacts in tropics or sanctuary.

I've checked that the computed offsets after all the depth offset alignment stuff in gen7_blorp.cpp matches what it should be.  I don't see any more interesting restrictions in the bspec.
Comment 3 Paul Berry 2012-08-22 14:33:32 UTC
Ken asked me to look at this while Eric was away on vacation, and I just figured out the problem this morning.

Unigine is executing the following sequence of operations on a cube map depth texture:

- Clear face 0
- Render to face 0
- Clear face 1
- Render to face 1
- Clear face 5
- Render to face 5
- Read from texture as a shadow map.

When clearing face N, brw_fast_clear_depth() calls intel_resolve_map_clear().  This tosses resolve map entries for all levels and layers of the texture, even though only face N was cleared.  As a result, after clearing face 5, the resolve map indicates that only face 5 needs a depth resolve, whereas in fact all faces need a depth resolve.

This is why we could work around the problem by bailing out of fast depth clear when depth_irb->mt_layer != 0: because it ensures that we only make the erroneous call to intel_resolve_map_clear() on the first clear, before touching any of the other faces.  If Unigine had rendered to the faces in a different order, this would not have worked.

Patch and piglit test to follow soon.
Comment 4 Paul Berry 2012-08-22 15:38:32 UTC
Patches sent to mesa-dev list for review:

[PATCH 1/2] i965/HiZ: remove assertion from intel_resolve_map_set().
[PATCH 2/2] i965: don't clear resolve map when doing fast depth clears.
Comment 5 Paul Berry 2012-08-24 17:40:36 UTC
A test to provoke this bug has been sent to the Piglit list for review:

[PATCH] texturing: add full "round trip" test of array depth textures.
Comment 6 Paul Berry 2012-08-24 17:41:10 UTC
Fixed by the following Mesa commits:

commit 5133bd6585552a5708b294180fa9a561bf7564a6
Author: Paul Berry <stereotype441@gmail.com>
Date:   Wed Aug 22 08:02:39 2012 -0700

    i965: don't clear resolve map when doing fast depth clears.
    Previously, when performing a fast depth clear, we would also clear
    the miptree's resolve map.  This destroyed important information,
    since the resolve map contains information about needed resolves for
    all levels and layers of the miptree, whereas a depth clear only
    applies to a single level/layer combination at a time.  As a result,
    resolves would sometimes fail to occur, leading to incorrect
    Fixes rendering artifacts with shadow maps in Unigine Heaven and
    Unigine Sanctuary.
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50270
    Reviewed-by: Eric Anholt <eric@anholt.net>
    Reviewed-by: Chad Versace <chad.versace@linux.intel.com>

commit 4b8b6f385e855ecb6da0b7dea56e70e69d1517b9
Author: Paul Berry <stereotype441@gmail.com>
Date:   Wed Aug 22 08:01:58 2012 -0700

    i965/HiZ: remove assertion from intel_resolve_map_set().
    There are three possible resolve map states for each (level, layer) of
    a depth miptree: "needs HiZ resolve", "needs depth resolve", and
    "needs neither".  When HiZ was first implemented on i965, any attempt
    to directly transition between "needs HiZ resolve" and "needs depth
    resolve" without passing through the "needs neither" state would have
    been a bug indicating that a necessary resolve hadn't been performed.
    Accordingly, intel_resolve_map_set() contained an assertion to verify
    that no such direct transition happened.
    However, now that we support fast depth clears, there is a valid
    transition from the "needs HiZ resolve" to the "needs depth resolve"
    state.  When doing a fast depth clear, the old state of the buffer is
    irrelevant, since we are completely replacing it with the clear value,
    so it is not necessary to do any resolves before clearing--we can
    transition, if necessary, directly from the "needs HiZ resolve" state
    to the "needs depth resolve" state.
    To avoid spurious assertions in this valid case, this patch just
    removes the assertion.
    Reviewed-by: Eric Anholt <eric@anholt.net>
    Reviewed-by: Chad Versace <chad.versace@linux.intel.com>

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.