[Mesa-dev] [PATCH] i965/blorp: Fix hiz ops on MSAA surfaces

[Chad Versace]

On Tue 18 Nov 2014, Chris Forbes wrote:
>Two things were broken here:
>- The depth/stencil surface dimensions were broken for MSAA.
>- Sample count was programmed incorrectly.
>
>Result was the depth resolve didn't work correctly on MSAA surfaces, and
>so sampling the surface later produced garbage.
>
>Fixes the new piglit test arb_texture_multisample-sample-depth, and
>various artifacts in 'tesseract' with msaa=4 glineardepth=0.
>
>Not observed any piglit regressions on Haswell.
>
>v2: Just set brw_hiz_op_params::dst.num_samples rather than adding a
>    helper function (Ken).
>
>Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>---
> src/mesa/drivers/dri/i965/brw_blorp.cpp  |  1 +
> src/mesa/drivers/dri/i965/gen7_blorp.cpp | 11 +++++++++++
> 2 files changed, 12 insertions(+)
>
>diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp b/src/mesa/drivers/dri/i965/brw_blorp.cpp
>index 20ce7b7..0ecf330 100644
>--- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
>+++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
>@@ -323,6 +323,7 @@ brw_hiz_op_params::brw_hiz_op_params(struct intel_mipmap_tree *mt,
>     */
>    depth.width = ALIGN(depth.width, 8);
>    depth.height = ALIGN(depth.height, 4);
>+   dst.num_samples = mt->num_samples;
>
>    x1 = depth.width;
>    y1 = depth.height;
>diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
>index 206a6ff..03fc9c8 100644
>--- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp
>+++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp
>@@ -663,6 +663,17 @@ gen7_blorp_emit_depth_stencil_config(struct brw_context *brw,
>        */
>       surfwidth = params->depth.width;
>       surfheight = params->depth.height;
>+
>+      if (params->dst.num_samples > 1) {
>+         /* If this is an MSAA + HIZ op, we need to program the
>+          * aligned logical size of the depth surface.
>+          */
>+         surfwidth = ALIGN(params->depth.mt->logical_width0, 8);
>+         surfheight = ALIGN(params->depth.mt->logical_height0, 4);
>+      } else {
>+         surfwidth = params->depth.width;
>+         surfheight = params->depth.height;
>+      }

I think the code that aligns up to 8x4 should go into hunk above that 
touches brw_hiz_op_params::brw_hiz_op_params(), because:

    1. That function already does the 8x4 alignment for the non-MSAA
       case. It doesn't make sense to do the 8x4 alignment for MSAA as
       a special case in a different file.
    2. The lengthy comment that explains how alignment works and why 
       it's needed is in that function. This is tricky delicate stuff, 
       so let's put the special cases near the documentation.

>    } else {
>       surfwidth = params->depth.mt->logical_width0;
>       surfheight = params->depth.mt->logical_height0;
>-- 
>2.1.3
>