[Mesa-dev] [PATCH 2/2] i965: Use IVB specific formula for depthbuffer

[Chad Versace]

On 09/22/2013 10:37 AM, Ben Widawsky wrote:
> After the last patch, we can replace the region allocated in the miptree
> creation with a more straightforward (and hopefully smaller resulting)
> buffer based on the bspec's allocation formula.
>
> Since I am relatively new to this part of the bspec, I would very much
> appreciate scrutiny during review of this. There were some ambiguities
> to me which are likely obvious to others.
>
> To prove the reduced [GPU] memory usage I created a simple script which
> polls the memory usage of the process through debugfs ever .1 seconds.
> The following results show the memory usage difference over 5 runs of
> xonotic-glx with ultra settings.
>
> The data suggests a 10MB savings on average. I've not measured the
> savings on the CPU side, but I imagine some amount of savings would be
> present there as well.
>
> x master/mem_usage.txt
> + mine/mem_usage.txt
>      N           Min           Max        Median           Avg Stddev
> x 17121      98959360 7.3394995e+08 7.2782234e+08 7.2209615e+08 43633222
> + 17166 1.2538266e+08 7.2241562e+08   7.16288e+08 7.1071472e+08 42964578
>
> Below is the FPS data over those same 5 tests. I'm not sure if the
> decrease is statistically significant to y'all. I don't have any
> theories about it.
>
> x master/xonotic.fps
> + mine/xonotic.fps
>      N           Min           Max        Median           Avg Stddev
> x   5     27.430746     27.524985      27.50568     27.487017 0.039439874
> +   5     27.409173     27.461715     27.441207     27.440883 0.021086805
>
> NOTE: There were a couple of places in the arithmetic where I could have
> taken some shortcuts. In order to make the code match with the spec as
> much as possible, I've decided not to do this. One shortcut I did make
> was the tiling type. Digging through the code it looks like you always
> want Y-tiled, except when it won't fit, in which case you want X-tiled.
> I wasn't a fan of the existing helper function that's there since it has
> a few irrelevant parameters for this operation. I suspect people
> reviewing this might ask me to change this, which is fine; I just wanted
> to explain the motivation.
>
> CC: Chad Versace <chad.versace at linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67564
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>   src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 149 +++++++++++++++++++++++---
>   1 file changed, 133 insertions(+), 16 deletions(-)


I began reviewing the equations in this patch, and hit a nasty roadblock.
The all-important variables z_width, z_height, and z_depth are the problem.
My interpretation of what those variables should be *differs* from how
the driver currently programs them in 3DSTATE_DEPTH_BUFFER. I believe the
driver has a bug regarding this.

Until I determine who is correct, the driver or me, I'm unable to
review your implementation of the equations.

Let's put this patch on hold for several days while I write a Piglit
test to determine the truth.