Bug 101678

Summary:	5-10% drop in Lightsmark2008 from "i965/miptree: Store fast clear colors in an isl_color_value"
Product:	Mesa	Reporter:	Eero Tamminen <eero.t.tamminen>
Component:	Drivers/DRI/i965	Assignee:	Jason Ekstrand <jason>
Status:	VERIFIED FIXED	QA Contact:	Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity:	normal
Priority:	medium
Version:	git
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:

Description Eero Tamminen 2017-07-03 14:57:09 UTC

Lightsmark2008 performance dropped by 5-10% on all machines (GEN6+) on Jun 7th.  EzBench bisected that to:
----------------------------------------------------------
AuthorDate: Sat May 20 15:00:42 2017 -0700
Commit:     Jason Ekstrand <jason.ekstrand@intel.com>
CommitDate: Wed Jun 7 08:54:54 2017 -0700

    i965/miptree: Store fast clear colors in an isl_color_value
    
    This commit, out of necessity, makes a number of changes at once:
    
     1) Changes intel_mipmap_tree to store the clear color for both color
        and depth as an isl_color_value.
    
     2) Changes the depth/stencil emit code to do the format conversion of
        the depth clear value on Haswell and earlier instead of pulling a
        uint32_t directly from the miptree.
    
     3) Changes ISL's depth/stencil emit code to perform the format
        conversion of the depth clear value on Haswell and earlier instead
        of assuming that the depth value in the float is pre-converted.
    
     4) Changes blorp to pass the depth value through as a float.
    
     5) Changes the Vulkan driver to pass the depth value to blorp as a
        float rather than a uint.
    
    Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
    Reviewed-by: Chad Versace <chadversary@chromium.org>
----------------------------------------------------------

No other benchmarks besides Lightsmark2008 were affected.

Comment 1 Jason Ekstrand 2017-08-20 03:57:56 UTC

This patch should take care of it:

https://patchwork.freedesktop.org/patch/172642/

Unfortunately, I can't really check right now because my laptop thermally throttles too bad.

Comment 2 Eero Tamminen 2017-08-21 11:30:50 UTC

On quick testing, that patch doesn't have any effect.


The regression went away already earlier, with this commit:
---------------------------------------------
commit 6db193701e7d03e0eadd54fc3e1b3d75719bc4ae
Author:     Jason Ekstrand <jason@jlekstrand.net>
AuthorDate: Wed Jun 14 18:54:28 2017 -0700
Commit:     Kenneth Graunke <kenneth@whitecape.org>
CommitDate: Wed Jul 26 14:43:01 2017 -0700

    i965: Only do depth resolves prior to clearing when needed
    
    When changing the clear value, we need to resolve any fast cleared data.
    
    Previously, we were performing resolves on every slice with HiZ enabled.
    We only need to resolve slices that a) have fast clear data, and b)
    aren't about to be cleared to the new color.  In the latter case, we
    were actually doing a resolve, and then a fast clear - when we could
    skip both, causing the existing fast cleared area to be updated to the
    new clear value for no additional work.
    
    This patch stops using intel_miptree_prepare_access in favor of a more
    optimal open coded loop that knows about our clear operation.
    
    v2: (by Ken) Rebase on islification, write a real commit message.
---------------------------------------------

(I bisected it last week, but hadn't yet had time to update this bug.  That change didn't effect other benchmarks.)

If you think that's enough, please mark this bug as fixed.

Comment 3 Eero Tamminen 2017-08-21 14:50:31 UTC

(In reply to Eero Tamminen from comment #2)
> On quick testing, that patch doesn't have any effect.

After longer testing, impact on other test-cases than Lightsmark is also within normal variation.

Comment 4 Jason Ekstrand 2017-08-21 16:00:38 UTC

I'm not surprised that the "fix" bisects to that patch.  It should fix the same underlying performance issue (the added resolves) but for completely different reasons.  The patch I sent over the weekend reverses the change from May and fixes the issue that way.  I think my new patch is still needed as it will matter in the multi-slice case (it mattered in all cases back in may).  I'd like to verify that my patch on top of the "bad" commit in May restores the original performance.

Comment 5 Jason Ekstrand 2017-08-21 23:23:38 UTC

I've verified on my KBL GT2 that we get a ~8% drop with the patch in may and that my patch on the list restores the original performance.  That's good enough for me to merge it (pending review)

Comment 6 Eero Tamminen 2017-08-23 10:47:56 UTC

Ok, the new patch is also in upstream now, I'll mark this as fixed.

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.