Bug 99850 - Tessellation bug on Carrizo
Summary: Tessellation bug on Carrizo
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/radeonsi (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Default DRI bug account
QA Contact: Default DRI bug account
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-17 15:32 UTC by Tom St Denis
Modified: 2017-02-28 15:50 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
carrizo run with HEAD (3.05 MB, text/plain)
2017-02-17 15:32 UTC, Tom St Denis
Details
Carrizo run with HEAD~ (3.06 MB, text/plain)
2017-02-17 15:33 UTC, Tom St Denis
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom St Denis 2017-02-17 15:32:28 UTC
Created attachment 129692 [details]
carrizo run with HEAD

Tessellation is broken on Carrizo (A12-9800) hardware and apparently has been for a while.  The offending commit is 

commit a4e2146a9d24592ed7e3bf778e3c21c6cfb89330
Author: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Date:   Mon May 2 14:55:52 2016 +0200

    radeonsi: Use buffer loads and stores for passing data from TCS to TES.

    We always try to use 4-component loads, as LLVM does not combine loads
    and they bypass the L1 cache.

    We can't use a similar strategy for stores and this is especially
    notable with the tess factors, as they are often set with separate
    MOV's per component in the TGSI.

    We keep storing to LDS and the LDS space, so we can load the outputs
    later, either due to the shader, of for wrting the tess factors.

    Signed-off-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
    Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
    Reviewed-by: Marek Olšák <marek.olsak@amd.com>

I found it via running Unigine-Heaven with tessellation enabled.

The bug doesn't occur on other VI hardware (Polaris10, FIJI).

I can confirm that the HEAD~ commit leads to spec/arb_tessellation_shader/* tests (in piglit) passing compared to running with HEAD.
Comment 1 Tom St Denis 2017-02-17 15:33:04 UTC
Created attachment 129693 [details]
Carrizo run with HEAD~
Comment 2 Tom St Denis 2017-02-17 17:53:24 UTC
Dug up the original thread that introduced this patch.  Might prove useful in debugging it.

https://lists.freedesktop.org/archives/mesa-dev/2016-May/116152.html
Comment 3 Michel Dänzer 2017-02-20 05:58:04 UTC
Bas, any ideas?
Comment 4 Tom St Denis 2017-02-22 17:11:40 UTC
Alex commented that mesa could use this addition to the kmd to selectively enable/disable this feature on APUs

commit 921e71c548b313f704745dcc609ffb0206da1afe                  
Author:     Junwei Zhang <Jerry.Zhang@amd.com>                   
AuthorDate: Fri Feb 17 11:05:49 2017 +0800                       
Commit:     Junwei Zhang <Jerry.Zhang@amd.com>                   
CommitDate: Tue Feb 21 10:22:30 2017 +0800                       

    drm/amdgpu: export gfx config double offchip LDS buffers (v3)

    v2: move the config struct to drm_amdgpu_info_device
    v3: move the config feature to amdgpu_gca_config

    Change-Id: I9651ab3e06a2d3c891b4c52a5da5b4a45d5809e5
    Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>            
    Reviewed-by: Christian König <christian.koenig@amd.com>
Comment 5 Alex Deucher 2017-02-22 17:14:12 UTC
(In reply to Tom St Denis from comment #4)
> Alex commented that mesa could use this addition to the kmd to selectively
> enable/disable this feature on APUs

Not to selectively enable, but I think it might be related.  We may need some adjustments in the UMD to deal with the hw differences.
Comment 6 Marek Olšák 2017-02-22 17:15:00 UTC
You can try this diff:

diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c b/src/gallium/drivers/radeonsi/si_state_shaders.c
index f615aa8..750cdd6 100644
--- a/src/gallium/drivers/radeonsi/si_state_shaders.c
+++ b/src/gallium/drivers/radeonsi/si_state_shaders.c
@@ -2312,7 +2312,9 @@ static bool si_update_spi_tmpring_size(struct si_context *sctx)
 
 static void si_init_tess_factor_ring(struct si_context *sctx)
 {
-       bool double_offchip_buffers = sctx->b.chip_class >= CIK;
+       bool double_offchip_buffers = sctx->b.chip_class >= CIK &&
+                                     sctx->b.family != CHIP_CARRIZO &&
+                                     sctx->b.family != CHIP_STONEY;
        unsigned max_offchip_buffers_per_se = double_offchip_buffers ? 128 : 64;
        unsigned max_offchip_buffers = max_offchip_buffers_per_se *
                                       sctx->screen->b.info.max_se;
Comment 7 Tom St Denis 2017-02-22 17:20:08 UTC
I'll try this momentarily (rig is configured for polaris10).

Probably cleaner though to check the flag from the KMD though in the long run right?
Comment 8 Tom St Denis 2017-02-22 17:28:33 UTC
With that diff applied unigine-heaven works with tess enabled on the tip of mesa{master} as of right now.
Comment 9 Tom St Denis 2017-02-22 18:14:56 UTC
Confirmed 1759 "pass"'es with tessellation tests in piglit vs 1724 passes with HEAD~.

Yipee.
Comment 10 Michel Dänzer 2017-02-23 01:20:18 UTC
Bug reports should only be resolved once the fix has landed in Git, preferably with a reference to the commit fixing it.

BTW, always specify explicit Git commit hashes instead of "HEAD". HEAD refers to the currently checked out commit, it has no meaning outside of your local repository.
Comment 11 Tom St Denis 2017-02-23 10:34:42 UTC
Thanks Michel, I was a bit quick in closing the bug.

Note the bug description mentions the offending commit hash so instead of pasting that everywhere I called that HEAD and the good commit HEAD~.  Conventions I suppose... :-)
Comment 12 Samuel Pitoiset 2017-02-28 15:50:21 UTC
Should be fixed by 35915af6c9ab4bdc0f1f8584ca346602405bd7e4.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.