[Mesa-dev] [PATCH] i965: Change 8x multisample positions

[Anuj Phogat]

There are no standard sample positions defined in OpenGL and OpenGL
ES specs. Implementations have the freedom to pick the positions
which give plausible results. But the Vulkan 1.0 spec does define
standard sample positions for different sample counts. Defined
positions in Vulkan for all the sample counts except 8X match with
the positions we set in i965. We have an upcoming plan to share the
blorp code between OpenGL and Vulkan driver in near future. Keeping
the 8X sample positions same on both the drivers will help us move
in that direction.

Here is an argument by Neil Roberts (from commit 20250e85) against
any advantage of current 8X sample positions over the new ones:

"The comment above for the 8x sample positions says that the hardware
implements centroid interpolation by picking the centre-most sample
that is inside the primitive. That implies that it might be worthwhile
to pick a pattern that includes 0.5,0.5. However by experimentation
this doesn't seem to actually be the case. With the sample positions
in this patch, if I modify the piglit test below so that it instead
reports the centroid position, it reports 0.492188,0.421875 which
doesn't match any of the positions. If I modify the sample positions
so that they include one at exactly 0.5,0.5 it doesn't help and it
reports another position which is even further from the center for
some reason.

arb_gpu_shader5-interpolateAtSample-different

Kenneth Graunke experimented with some other patterns that have a
higher standard deviation but I think after some discussion it was
decided that it would be better to pick the same pattern as the other
graphics API in case there are games that rely on this pattern."

Observed no regressions in jenkins testing.

Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
Cc: Jason Ekstrand <jason at jlekstrand.net>
---
 src/mesa/drivers/dri/i965/brw_multisample_state.h | 33 ++++++++---------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_multisample_state.h b/src/mesa/drivers/dri/i965/brw_multisample_state.h
index 42a7fd3..78fb03a 100644
--- a/src/mesa/drivers/dri/i965/brw_multisample_state.h
+++ b/src/mesa/drivers/dri/i965/brw_multisample_state.h
@@ -46,20 +46,9 @@ static const uint32_t
 brw_multisample_positions_4x = 0xae2ae662;
 
 /**
- * Sample positions are based on a solution to the "8 queens" puzzle.
- * Rationale: in a solution to the 8 queens puzzle, no two queens share
- * a row, column, or diagonal.  This is a desirable property for samples
- * in a multisampling pattern, because it ensures that the samples are
- * relatively uniformly distributed through the pixel.
- *
- * There are several solutions to the 8 queens puzzle (see
- * http://en.wikipedia.org/wiki/Eight_queens_puzzle).  This solution was
- * chosen because it has a queen close to the center; this should
- * improve the accuracy of centroid interpolation, since the hardware
- * implements centroid interpolation by choosing the centermost sample
- * that overlaps with the primitive being drawn.
+ * Sample positions:
  *
- * Note: from the Ivy Bridge PRM, Vol2 Part1 p304 (3DSTATE_MULTISAMPLE:
+ * From the Ivy Bridge PRM, Vol2 Part1 p304 (3DSTATE_MULTISAMPLE:
  * Programming Notes):
  *
  *     "When programming the sample offsets (for NUMSAMPLES_4 or _8 and
@@ -70,17 +59,17 @@ brw_multisample_positions_4x = 0xae2ae662;
  *
  * Sample positions:
  *   1 3 5 7 9 b d f
- * 1     5
- * 3           2
- * 5               6
- * 7 4
- * 9       0
- * b             3
- * d         1
- * f   7
+ * 1               7
+ * 3     3
+ * 5         0
+ * 7 5
+ * 9             2
+ * b       1
+ * d   4
+ * f           6
  */
 static const uint32_t
-brw_multisample_positions_8x[] = { 0xdbb39d79, 0x3ff55117 };
+brw_multisample_positions_8x[] = { 0x53d97b95, 0xf1bf173d };
 
 /**
  * Sample positions:
-- 
2.5.5